Rails7 test fixes #9231

jrafanie · 2024-07-25T20:52:03Z

This is ready for review but depends on rails 7.

Note, this will only pass on rails 7 and is included in the core cross repo:

[BOT] Cross repo test for ManageIQ/manageiq#22873 manageiq-cross_repo-tests#871

Depends:

Rails 7 manageiq#22873

kbrock · 2024-08-01T02:46:24Z

spec/controllers/vm_cloud_controller_spec.rb

-        :model_name                     => 'ManageIQ::Providers::CloudManager::Vm',
+        :model_name                     => 'VmOrTemplate',


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.

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.

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

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

The test framework changed, this may be just a testing change.

The expectation for instance variables in controllers may have changed and this may pose a problem in production.

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?

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

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.

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

kbrock · 2024-08-01T13:04:12Z

spec/controllers/vm_infra_controller_spec.rb

@@ -74,8 +74,8 @@

  # 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)
+    vm_vmware.ems_custom_attributes = [custom_attr1, custom_attr2]


ems_custom_attributes is an association.
I think << may be a better approach

ok, so the issue is the custom attribute was created with a nil source.
If you create the attribute with :source => "VC", then this works great.

I do remember warnings that scopes will not properly pass across for collection.create. I am not sure if this is related.

WAT. That's crazy. So, why does it work with assignment?

Nice find. It's interesting that I still need the identify_record twice change below.

jrafanie · 2024-08-02T14:13:02Z

spec/controllers/vm_infra_controller_spec.rb

@@ -65,7 +65,7 @@

  let(:custom_attr2) do
    FactoryBot.create(
-      :custom_attribute,
+      :ems_custom_attribute,


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

jrafanie · 2024-08-02T14:13:17Z

spec/controllers/vm_infra_controller_spec.rb

@@ -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)


This is still needed.

agreed. I saw the same thing

It's was unclear at first why we don't hit this in rails 6.1 because on rails 7, we get VmOrTemplate as I would expect from this code: https://github.com/ManageIQ/manageiq-ui-classic/blob/9761b105b3a0038f30c1b933c24bee99ea3daf0b/app/controllers/mixins/actions/vm_actions/ownership.rb#L71-L86 Keenan made the following findings: 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.

Without this, the source is set to nil and the ems custom attribute may not be persisted correctly, causing a test failure.

miq-bot · 2024-08-02T14:30:33Z

Checked commits jrafanie/manageiq-ui-classic@04ce526~...63dae15 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 🍰

jrafanie added wip rails7 labels Jul 25, 2024

jrafanie requested a review from a team as a code owner July 25, 2024 20:52

jrafanie mentioned this pull request Jul 25, 2024

Upgrade to rails 7.0 ManageIQ/manageiq#22052

Closed

miq-bot removed the wip label Jul 25, 2024

Fryguy changed the title ~~[WIP[ Rails7 test fixes~~ [WIP] Rails7 test fixes Jul 26, 2024

miq-bot added the wip label Jul 26, 2024

kbrock reviewed Aug 1, 2024

View reviewed changes

jrafanie force-pushed the rails7_test_fixes branch from fd2d283 to aaf37a0 Compare August 2, 2024 14:12

jrafanie commented Aug 2, 2024

View reviewed changes

jrafanie added 3 commits August 2, 2024 10:28

Set "source" on the ems custom attribute by using the factory

9a50664

Without this, the source is set to nil and the ems custom attribute may not be persisted correctly, causing a test failure.

For some reason, we're calling this method twice in rails 7

63dae15

jrafanie force-pushed the rails7_test_fixes branch from aaf37a0 to 63dae15 Compare August 2, 2024 14:28

jrafanie changed the title ~~[WIP] Rails7 test fixes~~ Rails7 test fixes Aug 2, 2024

jrafanie removed the wip label Aug 2, 2024

jrafanie mentioned this pull request Aug 2, 2024

Rails 7 ManageIQ/manageiq#22873

Merged

3 tasks

Fryguy closed this Aug 6, 2024

Fryguy reopened this Aug 6, 2024

Fryguy self-assigned this Aug 6, 2024

Fryguy merged commit 3e816bd into ManageIQ:master Aug 6, 2024
19 of 29 checks passed

jrafanie deleted the rails7_test_fixes branch August 6, 2024 17:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rails7 test fixes #9231

Rails7 test fixes #9231

jrafanie commented Jul 25, 2024 •

edited by Fryguy

Loading

kbrock Aug 1, 2024 •

edited

Loading

jrafanie Aug 1, 2024

kbrock Aug 2, 2024

jrafanie Aug 2, 2024

jrafanie Aug 2, 2024

jrafanie Aug 2, 2024

Fryguy Aug 6, 2024

kbrock Aug 1, 2024

kbrock Aug 2, 2024

jrafanie Aug 2, 2024

jrafanie Aug 2, 2024

jrafanie Aug 2, 2024

jrafanie Aug 2, 2024

kbrock Aug 2, 2024

miq-bot commented Aug 2, 2024

		:model_name => 'ManageIQ::Providers::CloudManager::Vm',
		:model_name => 'VmOrTemplate',

Rails7 test fixes #9231

Rails7 test fixes #9231

Conversation

jrafanie commented Jul 25, 2024 • edited by Fryguy Loading

kbrock Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Aug 2, 2024

jrafanie commented Jul 25, 2024 •

edited by Fryguy

Loading

kbrock Aug 1, 2024 •

edited

Loading