-
Notifications
You must be signed in to change notification settings - Fork 998
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 #37426 - Rubocop minitest rules fix #9977
Conversation
12d8105
to
dbd92f8
Compare
@ekohl we do want |
I'll raise another PR to split up the work for Lint and Style cops to fix. |
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.
Thanks, @archanaserver, few suggestions:
@archanaserver historically we've dealt with those bigger changes in separate PRs because it's hard to review multiple cops at the same time. Experience tells us that it's hard to get a consensus and forcing a consensus on all at the same time prolongs the review process a lot. That's why I suggested to split it. the minitest ones are not controversial and can be merged quickly. |
2f8031c
to
30dbe89
Compare
d33bb08
to
7ed9ae0
Compare
0b16917
to
48dfe92
Compare
I'm not sure why the unit tests fail. Have you taken a look? |
Yes, this is the error:
coming from here https://github.com/theforeman/foreman/blob/develop/test/factories/disable_auditing.rb#L13 after looking at it the uniqueness validation error pointing to https://github.com/theforeman/foreman/blob/develop/app/models/usergroup.rb#L31 so after looking at the log, i can say that there's an attempt to create a We need to find a way which ensure that each |
@ekohl any thoughts on this one? |
48dfe92
to
38a611e
Compare
@MariaAga, I've noticed there is a webpack failure here. Would you mind taking a look to see what's causing it? |
@archanaserver see #10034 for that. |
|
2acb930
to
38a611e
Compare
@ekohl what does that mean? edit : and what is stale review? i only reverted my last changes. |
e9927bf
to
1b6484b
Compare
@archanaserver, there are still two issues related to minitest: https://github.com/theforeman/foreman/actions/runs/8004471095/job/21861933764?pr=9977 |
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.
Please open a Redmine issue.
1b6484b
to
00c7467
Compare
@ekohl to pass the redmine check, should I mention same redmine issue number in all commit message or just squash it? |
You can use |
11b3313
to
2d44e9c
Compare
@ekohl done, thanks :) |
2d44e9c
to
f66a488
Compare
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.
Thanks, @archanaserver, LGTM, I'll leave the button to Ewoud just in case.
Refactored tests to utilize `assert_nil` for asserting nil values, in alignment with the `Minitest/AssertNil` cop.
Updated test to utilize refute_nil for asserting non-nil values.
f66a488
to
7fc49a4
Compare
upgrading theforeman-rubocop gem to v0.1.2 and that needs some rubocop rules fix.