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

rubocops/*: improve test descriptions #10334

Merged

Conversation

SeekingMeaning
Copy link
Contributor

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?
  • Have you successfully run brew man locally and committed any changes?

Continuation of #10124

This PR improves some test descriptions so that the error messages are more coherent

Before:
RuboCop::Cop::FormulaAudit::ComponentsOrder no on_os_block does not fail when there is no on_os block

After:
RuboCop::Cop::FormulaAudit::ComponentsOrder when formula has no OS-specific blocks reports no offenses

@BrewTestBot
Copy link
Member

Review period will end on 2021-01-18 at 03:08:47 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Jan 15, 2021
Copy link
Member

@Rylan12 Rylan12 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks, @SeekingMeaning!

One minor non-blocking suggestion

context "When auditing urls" do
it "with offenses" do
context "when auditing URLs" do
it "reports any offenses" do
Copy link
Member

Choose a reason for hiding this comment

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

Minor but maybe this should be:

Suggested change
it "reports any offenses" do
it "reports all offenses" do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure how to describe this as technically this doesn't report all offenses since there are the two others below this

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I see what you mean. What you just pushed looks great to me (plus bonus clarity because formulae isn't that descriptive of what it is)

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Jan 18, 2021
@BrewTestBot
Copy link
Member

Review period ended.

@SeekingMeaning SeekingMeaning merged commit 48e4487 into Homebrew:master Jan 18, 2021
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Feb 18, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Feb 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants