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

[requests/providers_spec.rb] Jansa approved spec #946

Merged
merged 1 commit into from
Oct 27, 2020

Conversation

NickLaMuro
Copy link
Member

@NickLaMuro NickLaMuro commented Oct 26, 2020

There has been a bit of a saga with these specs:

And it mostly ties in with some code that was updated for kasparov and up for aggregate_vm_cpus, but wasn't backported to jansa (which was probably the right choice).

However, it caused the specs to behave differently on the two branches, and thus fail on jansa. The nice part about these originally is that these specs very much replicated what we were seeing here:

#923 (comment)

But since that isn't valid for both branches, it is better to use specs that are suited for both branches.

Links

QA steps

  1. Pull down a local copy of this repo

  2. Checkout the merge commit prior to my fix from [Renderer] Fix duplicate includes with extra_cols #933 being added to master

    $ git checkout c2dc6c96
    
  3. Override the manageiq-api gem in bundler.d/

    override_gem 'manageiq-api', :path => '/Users/nicklamuro/code/redhat/manageiq-api'

From there, you should be able to repeat the testing you did for #923 but just change the attributes in the request to what is in the specs here:

/api/providers?expand=resources&attributes=name,total_vms,total_vms_and_templates

There has been a bit of a saga with these specs:

- ManageIQ#933 (comment)
- ManageIQ#933 (comment)

And it mostly ties in with some code that was updated for kasparov and
up for `aggregate_vm_cpus`, but wasn't backported to `jansa` (which was
probably the right choice).

However, it caused the specs to behave differently on the two branches,
and thus fail on `jansa`.  The nice part about these originally is that
these specs very much replicated what we were seeing here:

ManageIQ#923 (comment)

But since that isn't valid for both branches, it is better to use specs
that are suited for both branches.
@NickLaMuro
Copy link
Member Author

@miq-bot add_label test
@miq-bot assign @jrafanie

Hey LJ, I think you are best suited for double checking that these new specs are representative of what we ran into with #923 previously.

I need to re-run this spec on jansa just to make sure, but if you could test this prior to my fix in #933 and confirm that it is slow as well, that would be great!

@miq-bot
Copy link
Member

miq-bot commented Oct 26, 2020

Checked commit NickLaMuro@6d858a3 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. 🍰

@NickLaMuro
Copy link
Member Author

NickLaMuro commented Oct 26, 2020

@jrafanie also, here are some hopefully easier instructions for setting up an environment to test what I was talking about in #946 (comment)

  1. Pull down a local copy of this repo

  2. Checkout the merge commit prior to my fix from [Renderer] Fix duplicate includes with extra_cols #933 being added to master

    $ git checkout c2dc6c96
    
  3. Override the manageiq-api gem in bundler.d/

    override_gem 'manageiq-api', :path => '/Users/nicklamuro/code/redhat/manageiq-api'

From there, you should be able to repeat the testing you did for #923 but just change the attributes in the request to what is in the specs here:

/api/providers?expand=resources&attributes=name,total_vms,total_vms_and_templates

@chessbyte
Copy link
Member

/cc @simaishi

Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

I have confirmed that this PR recreates nearly the same exact problem from #923. I'm good with making the test master / jansa compatible to avoid pulling back other changes to jansa elsewhere for the purposes of a single set of tests.

@chessbyte chessbyte assigned chessbyte and unassigned jrafanie Oct 27, 2020
@chessbyte chessbyte merged commit 4303f09 into ManageIQ:master Oct 27, 2020
@NickLaMuro
Copy link
Member Author

Oh right, I forgot kasparov/yes would also be something we have to do now. Yes, this is needed to fix the jansa build, and should most definitely be brought to kasparov to be consistent with master/jansa.

simaishi pushed a commit that referenced this pull request Oct 27, 2020
[requests/providers_spec.rb] Jansa approved spec

(cherry picked from commit 4303f09)
@simaishi
Copy link
Contributor

Jansa backport details:

$ git log -1
commit 915333f2077b83bd22c0bbcfae33d4622148aca5
Author: Oleg Barenboim <chessbyte@gmail.com>
Date:   Tue Oct 27 09:59:13 2020 -0400

    Merge pull request #946 from NickLaMuro/jansa_approved_test

    [requests/providers_spec.rb] Jansa approved spec

    (cherry picked from commit 4303f090cfc8aea7dedd28d3b5998670f2054925)

simaishi pushed a commit that referenced this pull request Oct 27, 2020
[requests/providers_spec.rb] Jansa approved spec

(cherry picked from commit 4303f09)
@simaishi
Copy link
Contributor

Kasparov backport details:

$ git log -1
commit 70cb299b289f1d3f003d549b09aac2ecfff8fac7
Author: Oleg Barenboim <chessbyte@gmail.com>
Date:   Tue Oct 27 09:59:13 2020 -0400

    Merge pull request #946 from NickLaMuro/jansa_approved_test

    [requests/providers_spec.rb] Jansa approved spec

    (cherry picked from commit 4303f090cfc8aea7dedd28d3b5998670f2054925)

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