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

test/os/mac: add tests to validate pkg-config library versions #8400

Merged
merged 1 commit into from Aug 21, 2020

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Aug 19, 2020

  • 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 tests with your changes locally?

These tests assume the needed SDKs are correctly installed, i.e. brew doctor passes.
The CLT version installed should be the latest available for the running OS.
The tests do not check other OS versions beyond than the one the tests are being run on.

It is not possible to automatically check the following libraries for version updates:

  • libedit (incorrect LIBEDIT_MAJOR/MINOR in histedit.h)
  • uuid (not a standalone library)

Additionally, libffi version detection cannot be performed on systems running Mojave or earlier.

For indeterminable cases, consult https://opensource.apple.com for the version used.

@Bo98 Bo98 changed the title test/os/mac: add tests to validate library versions test/os/mac: add tests to validate pkg-config library versions Aug 19, 2020
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Very nice 👏🏻

@Bo98
Copy link
Member Author

Bo98 commented Aug 20, 2020

Tested (and passing) on:

  • macOS 10.15.6 Catalina
  • macOS 10.14.6 Mojave

I can test on 10.13 High Sierra on one of the CI machines at some point.
I will test on 11.0 Big Sur once brew install-bundler-gems is working. Sorbet is the blocker there and I'm working with upstream to get that solved.

@MikeMcQuaid
Copy link
Member

I can test on 10.13 High Sierra on one of the CI machines at some point.

Could consider making it one of these things that actually runs on the CI machines (like the brew doctor tests)?

@Bo98
Copy link
Member Author

Bo98 commented Aug 20, 2020

Possibly, though tests are mostly designed to ensure the pkg-config files are updated when things change in the OS, which won't happen for older OS versions.

I suppose it could be tested when the tests themselves change - if that is expected to happen.

@MikeMcQuaid
Copy link
Member

Yeh, I guess I was thinking a test like this could be an easy way of avoiding manual verification. Could even be a (optional) brew doctor one?

@Bo98
Copy link
Member Author

Bo98 commented Aug 20, 2020

The effects of a wrong version in the *.pc files are often extremely minor. I don't think we've ever had a bug report of anything caused by that.

Something in brew doctor might make sense, but it should definitely be optional. I don't think we gain anything from making users run it that other checks don't already cover.

@MikeMcQuaid
Copy link
Member

@Bo98 Yeh my main thoughts are:

  • we already have CI for brew doctor we could piggy-back off
  • this approach avoids having to install/run rspec for system-setup stuff
  • checking the presence of files on the system feels more doctory than brew testsy in hindsight

@Bo98
Copy link
Member Author

Bo98 commented Aug 20, 2020

checking the presence of files on the system feels more doctory than brew testsy in hindsight

To clarify: although this test does have an exist check (mostly as a sanity check), the point of the tests here aren't to check whether the SDK is installed correctly but rather the *.pc files shipped in brew are correct.

@Bo98
Copy link
Member Author

Bo98 commented Aug 20, 2020

I can remove that exist check - that would make sense.

@Bo98
Copy link
Member Author

Bo98 commented Aug 21, 2020

TIL that adding tests can decrease coverage.

Screenshot 2020-08-21 at 11 40 07

@MikeMcQuaid
Copy link
Member

@Bo98 Lol simplecov. 🚢!

@MikeMcQuaid MikeMcQuaid merged commit 5d05a05 into Homebrew:master Aug 21, 2020
@MikeMcQuaid
Copy link
Member

Thanks again @Bo98!

@Bo98 Bo98 deleted the pc-version-tests branch August 21, 2020 23:37
@Bo98
Copy link
Member Author

Bo98 commented Aug 22, 2020

Coverage thing seems to be because codecov is picking the wrong base commit. We generate the coverage report on 27559050c83a84fb64167e3518d076adb3e9d7ec, and that's the commit that gets sent to codecov. It's a merge commit of 86eca5a into 6cde372, generated by GitHub (ref/pull/8400/merge). While codecov knows to turn 27559050c83a84fb64167e3518d076adb3e9d7ec into 86eca5a instead, it incorrectly takes the parent of 86eca5a (af1e600) as the base commit rather than use 6cde372 that the merge commit based on.

@MikeMcQuaid
Copy link
Member

@Bo98 Good catch, may be either needs a fix in the codecov gem or it's something we can pass by tweaking an environment variable.

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 15, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 15, 2020
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.

None yet

3 participants