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

Fixup `Checkstyle` for local resolves. #6707

Merged
merged 1 commit into from Nov 1, 2018

Conversation

Projects
None yet
2 participants
@jsirois
Member

jsirois commented Oct 31, 2018

Previously the local resolve in tests was pointed to the wrong path.
This was masked on unstable master since there, version changes come
earlier than code changes, meaning the checker dist from the prior
release was being remotely resolved. On a version bump change this
masking was exposed since no remote dist of the just-bumped-version was
deployed.

Upon unmasking, a bug was also revealed in the production code
implementation of local resolves from sys.path.

Both are fixed.

Fixes #6706

@jsirois jsirois referenced this pull request Oct 31, 2018

Merged

Prepare 1.11.0rc1. #6703

@jsirois

This comment has been minimized.

Member

jsirois commented Oct 31, 2018

A side implication of this fix is that all 3 code paths for checker pex creation are tested even though that was not the intention. Leaving this unintentional coverage in-place here.

@jsirois jsirois requested review from stuhood and CMLivingston Oct 31, 2018

@stuhood stuhood added this to the 1.11.x milestone Oct 31, 2018

stuhood added a commit that referenced this pull request Oct 31, 2018

@jsirois

This comment has been minimized.

Member

jsirois commented Oct 31, 2018

A legit looking failure under python3: https://travis-ci.org/pantsbuild/pants/jobs/449009615
Working it ...

@jsirois

This comment has been minimized.

Member

jsirois commented Oct 31, 2018

Actually - this is great. Test now fails on #6706.
Two birds with one stone coming up...

@jsirois

This comment has been minimized.

Member

jsirois commented Oct 31, 2018

The fix here was small < 10 delta lines, taking a bit though to figure out how to pass the PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS through to the code under test... Getting close.

Fixup `Checkstyle` local resolves.
Previously the local resolve in tests was pointed to the wrong path.
This was masked on unstable master since there, version changes come
earlier than code changes, meaning the checker dist from the prior
release was being remotely resolved. On a version bump change this
masking was exposed since no remote dist of the just-bumped-version was
deployed.

Upon unmasking, a bug was also revealed in the production code
implementation of local resolves from sys.path.

Both are fixed.

Fixes #6706

@jsirois jsirois force-pushed the jsirois:checkstyle/fix_test branch from a13bc5e to 32b41e0 Oct 31, 2018

@jsirois jsirois changed the title from Fixup `CheckstyleTest` for local resolves. to Fixup `Checkstyle` for local resolves. Oct 31, 2018

@jsirois

This comment has been minimized.

Member

jsirois commented Oct 31, 2018

Alright - all fixed for real.

@stuhood

stuhood approved these changes Nov 1, 2018

Thanks a lot John!

@stuhood

This comment has been minimized.

Member

stuhood commented Nov 1, 2018

Only failure seems to be due to #6704. Merging. Thanks John!

@stuhood stuhood merged commit 9425ff7 into pantsbuild:master Nov 1, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

stuhood added a commit that referenced this pull request Nov 1, 2018

Fixup `Checkstyle` local resolves. (#6707)
Previously the local resolve in tests was pointed to the wrong path.
This was masked on unstable master since there, version changes come
earlier than code changes, meaning the checker dist from the prior
release was being remotely resolved. On a version bump change this
masking was exposed since no remote dist of the just-bumped-version was
deployed.

Upon unmasking, a bug was also revealed in the production code
implementation of local resolves from sys.path.

Both are fixed.

Fixes #6706

stuhood added a commit to twitter/pants that referenced this pull request Nov 1, 2018

stuhood added a commit to twitter/pants that referenced this pull request Nov 1, 2018

stuhood added a commit to twitter/pants that referenced this pull request Nov 1, 2018

@jsirois jsirois deleted the jsirois:checkstyle/fix_test branch Nov 1, 2018

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