Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rails7 test fixes #9231

Merged
merged 3 commits into from
Aug 6, 2024
Merged

Rails7 test fixes #9231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
04ce526
Ownership class for controller parameter should be VmOrTemplate
jrafanie Jul 25, 2024
9a50664
Set "source" on the ems custom attribute by using the factory
jrafanie Jul 25, 2024
63dae15
For some reason, we're calling this method twice in rails 7
jrafanie Jul 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions spec/controllers/vm_cloud_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,10 +179,10 @@
post :explorer
expect(response.status).to eq(200)
expect_any_instance_of(GtlHelper).to receive(:render_gtl).with match_gtl_options(
:model_name => 'ManageIQ::Providers::CloudManager::Vm',
:model_name => 'VmOrTemplate',
Copy link
Member

@kbrock kbrock Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This value is from: @report_data_additional_options[:model]
via -- ApplicationHelper#model_to_report_data

Locally, this passed in x_active_tree = :instance_tree and got the correct model back in vm_common.rb

So this was passing for me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this was passing for me.

With rails 7 or 6.1?

FYI, ui-classic was failing with cross repo + rails 7 without this change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so this calls post 2 times: explorer and x_button.
In the explorer call, the db/model is ManageIQ::Providers::CloudManager::Vm
In the x_button the db/model is VmOrTemplate.

The explorer call sets the @report_data_additional_options.model to the current model, which is ManageIQ::Providers::CloudManager::Vm.
The x_button call will reuse the @report_data_additional_options.model value if it is present.

In 6.1, the x_button has the instance variable set, so the model is ManageIQ::Providers::CloudManager::Vm
In 7.0, actionpack-7.0.8.4/lib/action_controller/metal/testing.rb
has a method clear_instance_variables_between_requests. So @report_data_additional_options is not set and it sets the model to the current model, which is VmOrTemplate.

Conclusions

  1. The test may be setup incorrectly and the model should be the same for both calls. If this is a bug, it was probably always present in the test but did not rear it's head
  2. The test framework changed, this may be just a testing change.
  3. The expectation for instance variables in controllers may have changed and this may pose a problem in production.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this "could" have some impact on our existing UI code. I see get_view and get_db_view processing the show list options if this is not set. If we relied on subsequent requests to have these values persisted over requests, we will likely set rebuilding the options with possibly new/different values on subsequent requests. cc @GilbertCherrie @Fryguy. Great find @kbrock. I've been stumped looking at this one the past few several days off and on.

manageiq-ui-classic/app/controllers/application_controller.rb

Line 1296 in 43566cf

process_show_list_options(options, db) if @report_data_additional_options.nil?

manageiq-ui-classic/app/controllers/application_controller.rb

Line 1099 in 43566cf

if !fetch_data && @report_data_additional_options.nil?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going forward, I think, this code is the right code but we'll have to merge this with the rails 7 branch as this will not pass on 6.1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, I'll summarize Keenan's findings and put it in the commit message as I will forget about the details in a few days or even hours.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrafanie Did you update the commit message? Just waiting for this before merge.

:selected_records => [vm_openstack_tmd.id],
:report_data_additional_options => {
:model => 'ManageIQ::Providers::CloudManager::Vm',
:model => 'VmOrTemplate',
:lastaction => 'show_list',
}
)
Expand Down
6 changes: 3 additions & 3 deletions spec/controllers/vm_infra_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@

let(:custom_attr1) do
FactoryBot.create(
:custom_attribute,
:ems_custom_attribute,
:resource => vm_vmware,
:name => 'Proč by si jeden nepokrad',
:value => 'jó, v tom je Pepa demokrat'
Expand All @@ -65,7 +65,7 @@

let(:custom_attr2) do
FactoryBot.create(
:custom_attribute,
:ems_custom_attribute,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the great sleuthing @kbrock. I changed this to use the ems custom attribute factory which sets source to VC.

:resource => vm_vmware,
:name => nil,
:value => 'a šikulovi má být dána šance'
Expand All @@ -75,7 +75,7 @@
# http://localhost:3000/vm_infra/show/10000000000449
it 'can display VM details for vm with ems_custom_attributes and a null attribute name' do
vm_vmware.ems_custom_attributes.push(custom_attr1, custom_attr2)
expect(controller).to receive(:identify_record).and_return(vm_vmware)
expect(controller).to receive(:identify_record).twice.and_return(vm_vmware)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed. I saw the same thing


get :show, :params => { :id => vm_vmware.id }
expect(response).to redirect_to(:action => 'explorer')
Expand Down
Loading