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.

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

Fix possible spec failure due to array ordering #18009

Merged
merged 2 commits into from
Sep 21, 2018

Conversation

carbonin
Copy link
Member

Introduced in bf66236

Example failure: https://travis-ci.org/ManageIQ/manageiq/jobs/431577113

  1) EvmApplication.queue_status groups zone together
     Failure/Error:
       expect(described_class.queue_status).to eq(
         [
           {
             "Zone"   => zone1.name,
             "Queue"  => "generic",
             "Role"   => nil,
             "method" => "X.x",
             "oldest" => tgt_time.strftime("%Y-%m-%d"),
             "count"  => 1,
           },
       expected: [{"Zone"=>"Zone 9", "Queue"=>"generic", "Role"=>nil, "method"=>"X.x", "oldest"=>"2018-09-21", "count"..."=>"Zone 10", "Queue"=>"generic", "Role"=>nil, "method"=>"X.x", "oldest"=>"2018-09-21", "count"=>1}]
            got: [{"Zone"=>"Zone 10", "Queue"=>"generic", "Role"=>nil, "method"=>"X.x", "oldest"=>"2018-09-21", "count...e"=>"Zone 9", "Queue"=>"generic", "Role"=>nil, "method"=>"X.x", "oldest"=>"2018-09-21", "count"=>1}]
       (compared using ==)
       Diff:
       @@ -1,10 +1,10 @@
       -[{"Zone"=>"Zone 9",
       +[{"Zone"=>"Zone 10",
          "Queue"=>"generic",
          "Role"=>nil,
          "method"=>"X.x",
          "oldest"=>"2018-09-21",
          "count"=>1},
       - {"Zone"=>"Zone 10",
       + {"Zone"=>"Zone 9",
          "Queue"=>"generic",
          "Role"=>nil,
          "method"=>"X.x",
     # ./spec/lib/tasks/evm_application_spec.rb:233:in `block (3 levels) in <top (required)>'

Backport status will be the same as #17987

Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

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

👍 LGTM

Although, it was introduced by the way strings with numbers are sorted and bf66236 just exposed that...

@carbonin
Copy link
Member Author

Although, it was introduced by the way strings with numbers are sorted

Ah. I see. Because "Zone 10" is sorted before "Zone 9" ...

Is the array from the method supposed to be sorted? Should that be part of the test? I'm sure we could come up with another way to fix spec so that we continue to test that aspect of the method as well ...

@bdunne
Copy link
Member

bdunne commented Sep 21, 2018

Is the array from the method supposed to be sorted?

I'm not sure, maybe @kbrock knows?

The method sorts the messages by zone name, so match_array isn't
really testing all of the functionality.
If we explicitly name the zones then we can be sure that the ordering
is what we expect.
@miq-bot
Copy link
Member

miq-bot commented Sep 21, 2018

Checked commits carbonin/manageiq@c9aba72~...e78ff10 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

@carbonin
Copy link
Member Author

Okay, pushed another commit for ordering that actually makes the spec closer to the original by creating the zones with the names they had before @bdunne 's change. That should make the ordering consistent.

@bdunne bdunne merged commit af58d33 into ManageIQ:master Sep 21, 2018
@bdunne bdunne added this to the Sprint 95 Ending Sep 24, 2018 milestone Sep 21, 2018
@bdunne
Copy link
Member

bdunne commented Sep 21, 2018

Merged for now since it's the same as it used to be (and removes the random test failures). It would still be good to know if it's supposed to be sorted or if the string sorting is good enough and we can switch to match_array in the future.

@carbonin
Copy link
Member Author

Oh, yeah, forgot to actually link the line that does the sorting. It's definitely supposed to be sorted by zone name.

@kbrock
Copy link
Member

kbrock commented Sep 24, 2018

ugh. late to the game - this looks great

@simaishi
Copy link
Contributor

@carbonin We're backporting #17987 to Gaprindashvili. Please add gaprindashvili/yes if this can be backported to Gaprindashvili as is.

@simaishi
Copy link
Contributor

Turned out this test doesn't exist in Gaprindashvili, removing gaprindashvilil/yes

@carbonin carbonin deleted the zone_queue_status_spec_ordering branch August 16, 2019 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants