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

Add tests for `FormulaAuditor#audit_deps` #3292

Merged
merged 1 commit into from Nov 10, 2017

Conversation

Projects
None yet
3 participants
@claui
Member

claui commented Oct 9, 2017

These tests cover a few aspects of the FormulaAuditor#audit_deps method. The main focus is the part where FormulaAuditor checks for dependencies on formulas which are tagged keg_only with the :provided_by_macos reason.

For this particular kind of keg_only formulas, we expect brew audit --new-formula to fail with a problem message like:

Dependency 'bc' may be unnecessary as it is provided by
macOS; try to build this formula without it.

For more details, see the relevant discussion:

[1] Homebrew/homebrew-core#14067 (comment)

[2] #3290 (comment)

Show outdated Hide outdated Library/Homebrew/test/dev-cmd/audit_spec.rb Outdated
Show outdated Hide outdated Library/Homebrew/test/dev-cmd/audit_spec.rb Outdated
Show outdated Hide outdated Library/Homebrew/test/dev-cmd/audit_spec.rb Outdated
Show outdated Hide outdated Library/Homebrew/test/dev-cmd/audit_spec.rb Outdated
@claui

This comment has been minimized.

Show comment
Hide comment
@claui

claui Oct 20, 2017

Member

When I run brew style locally, it says I should use the => syntax.

$ brew style Library/Homebrew/test/dev-cmd/audit_spec.rb
== Library/Homebrew/test/dev-cmd/audit_spec.rb ==
C: 60: 34: Use hash rockets syntax.
C: 66:  1: Block has too many lines. [361/144]
C:155: 50: Use hash rockets syntax.
C:187: 56: Use hash rockets syntax.
C:208: 50: Use hash rockets syntax.
C:208: 64: Use hash rockets syntax.
C:290: 50: Use hash rockets syntax.
C:304: 50: Use hash rockets syntax.
C:318: 50: Use hash rockets syntax.
C:337: 50: Use hash rockets syntax.

1 file inspected, 10 offenses detected

However, CI says I should use the new Hash syntax:

Offenses:
Library/Homebrew/test/dev-cmd/audit_spec.rb:224:49: C: Use the new Ruby 1.9 hash syntax.
          formula_auditor "foo", <<-EOS.undent, :new_formula => true
                                                ^^^^^^^^^^^^^^^
Library/Homebrew/test/dev-cmd/audit_spec.rb:256:49: C: Use the new Ruby 1.9 hash syntax.
          formula_auditor "foo", <<-EOS.undent, :new_formula => true
                                                ^^^^^^^^^^^^^^^
660 files inspected, 2 offenses detected

@MikeMcQuaid Any idea why there seem to be two different – and partially contradicting – style checks in place?

Member

claui commented Oct 20, 2017

When I run brew style locally, it says I should use the => syntax.

$ brew style Library/Homebrew/test/dev-cmd/audit_spec.rb
== Library/Homebrew/test/dev-cmd/audit_spec.rb ==
C: 60: 34: Use hash rockets syntax.
C: 66:  1: Block has too many lines. [361/144]
C:155: 50: Use hash rockets syntax.
C:187: 56: Use hash rockets syntax.
C:208: 50: Use hash rockets syntax.
C:208: 64: Use hash rockets syntax.
C:290: 50: Use hash rockets syntax.
C:304: 50: Use hash rockets syntax.
C:318: 50: Use hash rockets syntax.
C:337: 50: Use hash rockets syntax.

1 file inspected, 10 offenses detected

However, CI says I should use the new Hash syntax:

Offenses:
Library/Homebrew/test/dev-cmd/audit_spec.rb:224:49: C: Use the new Ruby 1.9 hash syntax.
          formula_auditor "foo", <<-EOS.undent, :new_formula => true
                                                ^^^^^^^^^^^^^^^
Library/Homebrew/test/dev-cmd/audit_spec.rb:256:49: C: Use the new Ruby 1.9 hash syntax.
          formula_auditor "foo", <<-EOS.undent, :new_formula => true
                                                ^^^^^^^^^^^^^^^
660 files inspected, 2 offenses detected

@MikeMcQuaid Any idea why there seem to be two different – and partially contradicting – style checks in place?

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Oct 20, 2017

Member

Any idea why there seem to be two different – and partially contradicting – style checks in place?

One is for (mostly formulae) taps, one is for Homebrew/brew internal code. You should just run brew style without arguments and that will match CI.

Member

MikeMcQuaid commented Oct 20, 2017

Any idea why there seem to be two different – and partially contradicting – style checks in place?

One is for (mostly formulae) taps, one is for Homebrew/brew internal code. You should just run brew style without arguments and that will match CI.

Add tests for `FormulaAuditor#audit_deps`
These tests cover a few aspects of the `FormulaAuditor#audit_deps`
method. The main focus is the part where FormulaAuditor checks for
dependencies on formulas which are tagged `keg_only` with the
`:provided_by_macos` reason.

For this particular kind of `keg_only` formulas, we expect
`brew audit --new-formula` to fail with a problem message like:

> Dependency 'bc' may be unnecessary as it is provided by
> macOS; try to build this formula without it.

For more details, see the relevant discussion:

[1] Homebrew/homebrew-core#14067 (comment)

[2] #3290 (comment)
@stale

This comment has been minimized.

Show comment
Hide comment
@stale

stale bot Nov 10, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale bot commented Nov 10, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label Nov 10, 2017

@claui

This comment has been minimized.

Show comment
Hide comment
@claui

claui Nov 10, 2017

Member

@MikeMcQuaid Anything else I can do to get this merged?

(There’s obviously nothing I can do to accommodate our drunk friend Codecov.)

Member

claui commented Nov 10, 2017

@MikeMcQuaid Anything else I can do to get this merged?

(There’s obviously nothing I can do to accommodate our drunk friend Codecov.)

@stale stale bot removed the stale label Nov 10, 2017

@claui

This comment has been minimized.

Show comment
Hide comment
@claui

claui Nov 10, 2017

Member

@MikeMcQuaid Btw, thanks for the review 👍

Member

claui commented Nov 10, 2017

@MikeMcQuaid Btw, thanks for the review 👍

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Nov 10, 2017

Member

Sorry @claui, didn't know this was 💚 now. Thanks for the PR!

Member

MikeMcQuaid commented Nov 10, 2017

Sorry @claui, didn't know this was 💚 now. Thanks for the PR!

@MikeMcQuaid MikeMcQuaid merged commit 219f969 into Homebrew:master Nov 10, 2017

2 of 3 checks passed

codecov/project 67.62% (-0.38%) compared to a2374cb
Details
codecov/patch Coverage not affected when comparing a2374cb...e0bb978
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@claui claui deleted the claui:add-audit-test-cases branch Nov 10, 2017

@claui

This comment has been minimized.

Show comment
Hide comment
@claui

claui Nov 10, 2017

Member

Hmm. The error message suggests that the test environment somehow failed to apply the ’String’ monkeypatch.

I wonder why that happens, and why the (Linux) Travis build didn’t catch that.

@amyspark Did the test failures occur when the tests were run manually over the command line? Or under some sort of CI?

Edit: Never mind, I meant to answer to a comment but that comment has disappeared.

Member

claui commented Nov 10, 2017

Hmm. The error message suggests that the test environment somehow failed to apply the ’String’ monkeypatch.

I wonder why that happens, and why the (Linux) Travis build didn’t catch that.

@amyspark Did the test failures occur when the tests were run manually over the command line? Or under some sort of CI?

Edit: Never mind, I meant to answer to a comment but that comment has disappeared.

@amyspark

This comment has been minimized.

Show comment
Hide comment
@amyspark

amyspark Nov 10, 2017

Member

@claui, sorry for the deletion! I removed it because the issue is being addressed in #3440.
In short: you used EOS.undent to write your casksformulae, but undent is only defined under MacOS (brew/Library/Homebrew/compat/extend/string.rb).

Member

amyspark commented Nov 10, 2017

@claui, sorry for the deletion! I removed it because the issue is being addressed in #3440.
In short: you used EOS.undent to write your casksformulae, but undent is only defined under MacOS (brew/Library/Homebrew/compat/extend/string.rb).

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.