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.

Sign up for GitHub

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

Jump to bottom

Fix some Minitest/ cops #11065

Merged
merged 3 commits into from
Jul 18, 2024
Merged

Fix some Minitest/ cops #11065

merged 3 commits into from
Jul 18, 2024

Conversation

archanaserver
Copy link
Contributor

@archanaserver archanaserver commented Jul 8, 2024 •
edited
Loading

What are the changes introduced in this pull request?

Based on: #11009

This pull request addresses the Minitest/GlobalExpectations RuboCop offense by replacing global expectation methods with the recommended non-global expectation methods.

https://docs.rubocop.org/rubocop-minitest/cops_minitest.html#minitestglobalexpectations

Considerations taken when implementing this change?

Verified that the changes do not impact existing functionality or cause syntax errors.

What are the testing steps for this pull request?

bundle exec rubocop

@archanaserver archanaserver changed the title xFix Minitest/GlobalExpectations cop Fix Minitest/GlobalExpectations cop Jul 8, 2024
@archanaserver archanaserver mentioned this pull request Jul 8, 2024
Merged
ianballou
ianballou reviewed Jul 9, 2024
View reviewed changes
Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

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

It looks like the underscore is confusing gettext for katello/spec/models/host_collection_spec.rb. Perhaps because the underscore is also used to mark translations? The cop guide seems to have some examples without underscores.

@archanaserver archanaserver force-pushed the fix-minitest/globalexpectations branch from 2d856dc to 095124f Compare July 11, 2024 07:02
@archanaserver
Copy link
Contributor Author

It looks like the underscore is confusing gettext for katello/spec/models/host_collection_spec.rb. Perhaps because the underscore is also used to mark translations? The cop guide seems to have some examples without underscores.

I have followed different style for that specific file you mentioned, but not sure if this is good to follow different style for different files. wdyt?

@ianballou
Copy link
Member

It looks like the underscore is confusing gettext for katello/spec/models/host_collection_spec.rb. Perhaps because the underscore is also used to mark translations? The cop guide seems to have some examples without underscores.

I have followed different style for that specific file you mentioned, but not sure if this is good to follow different style for different files. wdyt?

It might be best to avoid the underscores entirely if possible since that usually represents translations. I don't feel super strongly about it though since it's in tests only as far as I see.

@archanaserver archanaserver changed the title Fix Minitest/GlobalExpectations cop Fix some Minitest/ cops Jul 12, 2024
@archanaserver archanaserver force-pushed the fix-minitest/globalexpectations branch 2 times, most recently from 6da69f6 to c20e4e2 Compare July 12, 2024 12:56
@archanaserver
Copy link
Contributor Author

It looks like the underscore is confusing gettext for katello/spec/models/host_collection_spec.rb. Perhaps because the underscore is also used to mark translations? The cop guide seems to have some examples without underscores.

I have followed different style for that specific file you mentioned, but not sure if this is good to follow different style for different files. wdyt?

It might be best to avoid the underscores entirely if possible since that usually represents translations. I don't feel super strongly about it though since it's in tests only as far as I see.

Done! @ianballou

@ianballou
Copy link
Member

Looks like there are two failures left, I'm seeing katello/spec/models/provider_spec.rb mentioned.

@archanaserver archanaserver force-pushed the fix-minitest/globalexpectations branch from c20e4e2 to d0d422e Compare July 15, 2024 10:54
@archanaserver
Copy link
Contributor Author

Looks like there are two failures left, I'm seeing katello/spec/models/provider_spec.rb mentioned.

@ianballou not sure what this failure now, can you help me in this?

ianballou
ianballou requested changes Jul 16, 2024
View reviewed changes
spec/models/host_collection_spec.rb Outdated
end

it "should not allow unlimited_hosts=false and max_hosts to be nil at the same time" do
create = lambda do
HostCollection.create!(:name => "TestHostCollection", :organization => @org, :unlimited_hosts => false)
end
create.must_raise(ActiveRecord::RecordInvalid)
value { create }.must_raise(ActiveRecord::RecordInvalid)
Copy link
Member

Choose a reason for hiding this comment

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

I think this is the issue:

Suggested change
value { create }.must_raise(ActiveRecord::RecordInvalid)
value(create).must_raise(ActiveRecord::RecordInvalid)

@archanaserver archanaserver force-pushed the fix-minitest/globalexpectations branch from d0d422e to ad99939 Compare July 16, 2024 19:54
@ianballou
Copy link
Member

The latest test failures are pretty odd -- need to see what's going on there. Maybe in the meantime rebase if you haven't.

@ianballou
Copy link
Member

Please rebase once #11076 is merged.

@archanaserver archanaserver force-pushed the fix-minitest/globalexpectations branch from ad99939 to 7cc89c3 Compare July 18, 2024 06:50
ianballou
ianballou approved these changes Jul 18, 2024
View reviewed changes
Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @archanaserver !

@ianballou ianballou merged commit b94f81d into Katello:master Jul 18, 2024
26 of 28 checks passed
@archanaserver
Copy link
Contributor Author

Thanks for the review @ianballou!

@archanaserver archanaserver deleted the fix-minitest/globalexpectations branch July 18, 2024 14:05
@archanaserver archanaserver mentioned this pull request Jul 30, 2024
Closed
35 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ianballou ianballou ianballou approved these changes

Assignees
No one assigned
Labels
Waiting on contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@archanaserver @ianballou