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

Fixes #37426 - Rubocop minitest rules fix #9977

Merged
merged 4 commits into from
Jul 31, 2024

Conversation

archanaserver
Copy link
Contributor

upgrading theforeman-rubocop gem to v0.1.2 and that needs some rubocop rules fix.

@archanaserver archanaserver marked this pull request as draft January 3, 2024 20:32
@archanaserver archanaserver changed the title Fix cops Minitest/AssertEmptyLiteral Rubocop rules fix Jan 4, 2024
@archanaserver
Copy link
Contributor Author

@ekohl we do want Layout/ArgumentAlignment cop fix with EnforcedStyle: with_fixed_indentation here?

@archanaserver archanaserver marked this pull request as ready for review January 4, 2024 09:08
@archanaserver
Copy link
Contributor Author

I'll raise another PR to split up the work for Lint and Style cops to fix.

Copy link
Member

@ofedoren ofedoren left a 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:

.rubocop.yml Outdated Show resolved Hide resolved
.rubocop.yml Outdated Show resolved Hide resolved
@ekohl
Copy link
Member

ekohl commented Jan 4, 2024

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

@archanaserver archanaserver changed the title Rubocop rules fix Rubocop minitest rules fix Jan 9, 2024
.rubocop.yml Outdated Show resolved Hide resolved
.rubocop_todo.yml Outdated Show resolved Hide resolved
test/controllers/api/v2/users_controller_test.rb Outdated Show resolved Hide resolved
ekohl
ekohl previously approved these changes Jan 16, 2024
@ekohl
Copy link
Member

ekohl commented Jan 16, 2024

I'm not sure why the unit tests fail. Have you taken a look?

@archanaserver
Copy link
Contributor Author

I'm not sure why the unit tests fail. Have you taken a look?

Yes, this is the error:

Error:
UsergroupTest#test_0007_should create with valid usergroup:
ActiveRecord::RecordInvalid: Validation failed: Name has already been taken
test/factories/disable_auditing.rb:13:in `block (3 levels) in <top (required)>'
test/factories/disable_auditing.rb:13:in `block (2 levels) in <top (required)>'
test/models/usergroup_test.rb:69:in `block (2 levels) in <class:UsergroupTest>'
test/models/usergroup_test.rb:68:in `each'
test/models/usergroup_test.rb:68:in `block in <class:UsergroupTest>'

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 Usergroup with a name that has already been taken.

We need to find a way which ensure that each Usergroup created with this factory has a unique name. I'm still looking how we can encounter the validation error here. @ekohl am i getting it right, do you have any suggestion for the same?

@archanaserver
Copy link
Contributor Author

@ekohl any thoughts on this one?

@archanaserver
Copy link
Contributor Author

@MariaAga, I've noticed there is a webpack failure here. Would you mind taking a look to see what's causing it?

@ekohl
Copy link
Member

ekohl commented Feb 16, 2024

@archanaserver see #10034 for that.

@MariaAga
Copy link
Member

invalid session id error in the tests is not related to this pr

@archanaserver
Copy link
Contributor Author

archanaserver commented Feb 22, 2024

@ekohl what does that mean?
"@archanaserver archanaserver dismissed ekohl’s stale review via 38a611e"

edit : and what is stale review? i only reverted my last changes.

@archanaserver archanaserver force-pushed the rubocop-fix branch 2 times, most recently from e9927bf to 1b6484b Compare February 22, 2024 12:28
@ofedoren
Copy link
Member

@archanaserver, there are still two issues related to minitest: https://github.com/theforeman/foreman/actions/runs/8004471095/job/21861933764?pr=9977

Copy link
Member

@ekohl ekohl left a 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.

.rubocop.yml Outdated Show resolved Hide resolved
.rubocop.yml Outdated Show resolved Hide resolved
@archanaserver archanaserver changed the title Rubocop minitest rules fix Fixes #37426 - Rubocop minitest rules fix May 9, 2024
@archanaserver
Copy link
Contributor Author

@ekohl to pass the redmine check, should I mention same redmine issue number in all commit message or just squash it?

@ekohl
Copy link
Member

ekohl commented May 10, 2024

You can use Refs #37426 in the other commits and only keep Fixes in the last commit.

@archanaserver
Copy link
Contributor Author

@ekohl done, thanks :)

@archanaserver
Copy link
Contributor Author

@ofedoren @ekohl, can you take a look again? i believe it is ready to be merged!

Copy link
Member

@ofedoren ofedoren left a 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.
@ekohl ekohl merged commit d59c5c1 into theforeman:develop Jul 31, 2024
50 of 53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants