foreman icon indicating copy to clipboard operation
foreman copied to clipboard

Fixes #37426 - Rubocop minitest rules fix

Open archanaserver opened this issue 1 year ago • 14 comments

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

archanaserver avatar Jan 03 '24 20:01 archanaserver

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

archanaserver avatar Jan 04 '24 09:01 archanaserver

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

archanaserver avatar Jan 04 '24 12:01 archanaserver

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

ekohl avatar Jan 04 '24 14:01 ekohl

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

ekohl avatar Jan 16 '24 18:01 ekohl

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 avatar Jan 19 '24 20:01 archanaserver

@ekohl any thoughts on this one?

archanaserver avatar Feb 01 '24 06:02 archanaserver

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

archanaserver avatar Feb 16 '24 07:02 archanaserver

@archanaserver see #10034 for that.

ekohl avatar Feb 16 '24 13:02 ekohl

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

MariaAga avatar Feb 16 '24 14:02 MariaAga

@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 avatar Feb 22 '24 12:02 archanaserver

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

ofedoren avatar Feb 22 '24 15:02 ofedoren

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

archanaserver avatar May 09 '24 10:05 archanaserver

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

ekohl avatar May 10 '24 10:05 ekohl

@ekohl done, thanks :)

archanaserver avatar May 15 '24 06:05 archanaserver

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

archanaserver avatar Jul 23 '24 11:07 archanaserver