-
Notifications
You must be signed in to change notification settings - Fork 287
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
Fixes #19664 - fix tests after admin made unavailable in taxonomies #6800
Conversation
@ares, thanks for your PR! By analyzing the history of the files in this pull request, we identified @parthaa, @ehelms and @jlsherrill to be potential reviewers. |
There were the following issues with the commit message:
If you don't have a ticket number, please create an issue in Redmine. More guidelines are available in Coding Standards or on the Foreman wiki. This message was auto-generated by Foreman's prprocessor |
@@ -88,7 +88,7 @@ module FixtureTestCase | |||
|
|||
Setting::Content.load_defaults | |||
|
|||
@@admin = ::User.find(FIXTURES['users']['admin']['id']) | |||
@@admin = ::User.unscoped.find(FIXTURES['users']['admin']['id']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace class var @@admin with a class instance var.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope I can ignore this :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ares From @johnpmitsch on IRC - "can you add an exception for the class variable so rubocop ignores it?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added on top of module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so i think hound is throwing up an error here, we don't enable the class var cop for tests, and now rubocop is failing with:
W: Unnecessary disabling of Style/ClassVars.
in jenkins :(
It seems like hound doesn't honor .rubocop.yaml files in subdirectories: https://github.com/Katello/katello/blob/master/test/.rubocop.yml
hound--
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm going to open a PR to move this to the top level rubocop.yml so that the behavior is consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still failing:
01:56:22.484 /var/lib/workspace/workspace/test_katello_pull_request/slave/fast/plugin/test/katello_test_helper.rb:65:1: W: Unnecessary disabling of Style/ClassVars.
01:56:22.484 # rubocop:disable Style/ClassVars
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just remove that line, i think hound should now be fixed
Seems good to me and tests passed. The commit summary length can be fixed on merge. Thanks @ares |
There were the following issues with the commit message:
If you don't have a ticket number, please create an issue in Redmine. More guidelines are available in Coding Standards or on the Foreman wiki. This message was auto-generated by Foreman's prprocessor |
There were the following issues with the commit message:
If you don't have a ticket number, please create an issue in Redmine. More guidelines are available in Coding Standards or on the Foreman wiki. This message was auto-generated by Foreman's prprocessor |
There were the following issues with the commit message:
If you don't have a ticket number, please create an issue in Redmine. More guidelines are available in Coding Standards or on the Foreman wiki. This message was auto-generated by Foreman's prprocessor |
There's still one issue when combined with Foreman PR, it seems that some foreman task test fails, investigating. |
The problem is now on with foreman-tasks, opened theforeman/foreman-tasks#252 but there's not way to run tests in jenkins with PR from core, katello and foreman-tasks at the same time. I spoke with Ivan, the patch seems alright and the foreman-tasks release is planned for this afternoon, then I can rerun the tests which should confirm that this PR will keep Katello tests running after CVE PR is merged. |
ACK thanks @ares |
No description provided.