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

Added tests for os/mac/keg #3079

Merged
merged 1 commit into from Sep 27, 2017

Conversation

Projects
None yet
4 participants
@mansimarkaur
Contributor

mansimarkaur commented Aug 22, 2017

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

@MikeMcQuaid MikeMcQuaid requested a review from woodruffw Aug 23, 2017

@MikeMcQuaid

Tests are failing here:
https://travis-ci.org/Homebrew/brew/jobs/267164682#L343

@woodruffw could you review this PR?

@woodruffw

This comment has been minimized.

Show comment
Hide comment
@woodruffw

woodruffw Aug 23, 2017

Member

could you review this PR?

Sure, no problem.

Member

woodruffw commented Aug 23, 2017

could you review this PR?

Sure, no problem.

@mansimarkaur

This comment has been minimized.

Show comment
Hide comment
@mansimarkaur

mansimarkaur Aug 25, 2017

Contributor

Please review, @woodruffw
Thanks!

Contributor

mansimarkaur commented Aug 25, 2017

Please review, @woodruffw
Thanks!

Show outdated Hide outdated Library/Homebrew/test/os/mac/keg_spec.rb Outdated
@mansimarkaur

This comment has been minimized.

Show comment
Hide comment
@mansimarkaur

mansimarkaur Aug 26, 2017

Contributor

The tests are still failing on my local. Could you try it on your system, @mistydemeo and also re-run the travis build to ensure the tests are actually passing?

Contributor

mansimarkaur commented Aug 26, 2017

The tests are still failing on my local. Could you try it on your system, @mistydemeo and also re-run the travis build to ensure the tests are actually passing?

@mistydemeo

This comment has been minimized.

Show comment
Hide comment
@mistydemeo

mistydemeo Aug 26, 2017

Contributor

I can repro failures for Keg#change_install_name changes file's install name to given name and Keg#change_dylib_id changes file's dylib id to given id locally.

Contributor

mistydemeo commented Aug 26, 2017

I can repro failures for Keg#change_install_name changes file's install name to given name and Keg#change_dylib_id changes file's dylib id to given id locally.

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Sep 5, 2017

Member

@woodruffw How's this looking to you now?

Member

MikeMcQuaid commented Sep 5, 2017

@woodruffw How's this looking to you now?

@woodruffw

This comment has been minimized.

Show comment
Hide comment
@woodruffw

woodruffw Sep 5, 2017

Member

@MikeMcQuaid I'm a bit stumped on the tests here - the only thing I can think of that would cause this behavior should be fixed in #3101, but @mansimarkaur rebased and confirmed that the tests are still failing.

I'm going to merge #3101 anyways since it'll bring a minor performance improvement with no breaking (public) changes, but otherwise those two failing tests might need to be stubbed out until I trace it down fully.

Member

woodruffw commented Sep 5, 2017

@MikeMcQuaid I'm a bit stumped on the tests here - the only thing I can think of that would cause this behavior should be fixed in #3101, but @mansimarkaur rebased and confirmed that the tests are still failing.

I'm going to merge #3101 anyways since it'll bring a minor performance improvement with no breaking (public) changes, but otherwise those two failing tests might need to be stubbed out until I trace it down fully.

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Sep 5, 2017

Member

@woodruffw Sounds good. Shout after that and we can rerun here and remove the tests if needed.

Member

MikeMcQuaid commented Sep 5, 2017

@woodruffw Sounds good. Shout after that and we can rerun here and remove the tests if needed.

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Sep 25, 2017

Member

@woodruffw What's up, here?

Member

MikeMcQuaid commented Sep 25, 2017

@woodruffw What's up, here?

@woodruffw

This comment has been minimized.

Show comment
Hide comment
@woodruffw

woodruffw Sep 25, 2017

Member

#3139 covers this, I just haven't made time to add some unit tests for it yet.

I'll have that PR done tonight, and I think that should fix the behavior here. If not, then commenting those tests out for the time being is good by me (since they don't test the relocation behavior we currently use, only potential combinatons).

Member

woodruffw commented Sep 25, 2017

#3139 covers this, I just haven't made time to add some unit tests for it yet.

I'll have that PR done tonight, and I think that should fix the behavior here. If not, then commenting those tests out for the time being is good by me (since they don't test the relocation behavior we currently use, only potential combinatons).

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Sep 25, 2017

Member

I'll have that PR done tonight, and I think that should fix the behavior here. If not, then commenting those tests out for the time being is good by me (since they don't test the relocation behavior we currently use, only potential combinatons).

👍. I think it's worth deleting rather than commenting out the tests. Would you mind doing that here and just merging when it's and you're happy with it? Thanks!

Member

MikeMcQuaid commented Sep 25, 2017

I'll have that PR done tonight, and I think that should fix the behavior here. If not, then commenting those tests out for the time being is good by me (since they don't test the relocation behavior we currently use, only potential combinatons).

👍. I think it's worth deleting rather than commenting out the tests. Would you mind doing that here and just merging when it's and you're happy with it? Thanks!

@woodruffw

This comment has been minimized.

Show comment
Hide comment
@woodruffw

woodruffw Sep 25, 2017

Member

No problem!

Member

woodruffw commented Sep 25, 2017

No problem!

@woodruffw woodruffw merged commit a1140f7 into Homebrew:master Sep 27, 2017

3 checks passed

codecov/patch Coverage not affected when comparing 24d7475...9f57840
Details
codecov/project 67.15% (+0.1%) compared to 24d7475
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

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