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

Port BaseTest to v2 engine #4867

Merged
merged 12 commits into from Mar 12, 2018

Conversation

Projects
None yet
4 participants
@stuhood
Copy link
Member

stuhood commented Sep 13, 2017

Problem

The v2 engine is now enabled by default as the backend for BUILD file parsing and file fingerprinting, but due to test-specific setup referencing legacy code paths, all unit tests still use v1.

Additionally, there has been a longstanding TODO about renaming BaseTest to TestBase.

Solution

Add TestBase which uses v2 code paths, deprecate BaseTest (which continues to use v1), and migrate the vast majority of tests to TestBase.

Result

Users will have a deprecation cycle to port from BaseTest to TestBase and take on the relatively minor behaviour changes that come along with that.

@stuhood stuhood force-pushed the twitter:stuhood/engine-in-base-test branch 2 times, most recently from f163dc4 to 71a4426 Sep 14, 2017

@stuhood stuhood force-pushed the twitter:stuhood/engine-in-base-test branch from ca7c7ff to 2f365df Sep 26, 2017

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Oct 25, 2017

It occurs to me that this might be an obvious opportunity to finally fix the BaseTest/TestBase snafu, and deprecate one of them.... that would hopefully allow this to land much sooner.

@stuhood stuhood force-pushed the twitter:stuhood/engine-in-base-test branch from 64fe2a0 to 8c02f0d Mar 3, 2018

@stuhood stuhood force-pushed the twitter:stuhood/engine-in-base-test branch 2 times, most recently from 40f0e71 to 6b566f1 Mar 7, 2018

@stuhood stuhood requested review from jsirois , benjyw and kwlzn Mar 7, 2018

@stuhood stuhood force-pushed the twitter:stuhood/engine-in-base-test branch from 6b566f1 to 4032927 Mar 7, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Mar 7, 2018

There are a small number of test failures still to sort out, but this should now be ready for review.

You should be able to completely ignore the second commit, as it represents the find-replace of the various BaseTest->TestBase usages.

@stuhood stuhood changed the title WIP: Port BaseTest to v2 engine Port BaseTest to v2 engine Mar 7, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Mar 7, 2018

Fixed title.

with self.assertRaisesRegexp(BuildGraph.TransitiveLookupError,
'^.*/non-existent-path does not contain any BUILD files.'
'\s+when translating spec non-existent-path:b'
'\s+referenced from //:a$'):

This comment has been minimized.

@illicitonion

illicitonion Mar 7, 2018

Contributor

The "referenced from" was a useful assert which shouldn't be removed (or at least should have a TODO to re-add)

This comment has been minimized.

@stuhood

stuhood Mar 8, 2018

Member

Skipped with #4515

self.inject_address_closure('//:a')

@unittest.skip(reason='TODO: #4515')

This comment has been minimized.

@illicitonion

illicitonion Mar 7, 2018

Contributor

Do we have a plan to pick this up soon?

Regressing usability feels bad...

This comment has been minimized.

@stuhood

stuhood Mar 8, 2018

Member

Yea, I think that SelectTransitive will need to go one way or another... at which point we'll get back our dependency callstack.

BuildGraph.TransitiveLookupError,
'^Addresses in dependencies must be unique. \'other:b\' is referenced more than once.'
'\s+referenced from //:a$'):
with self.assertRaisesRegexp(AddressLookupError, '^Addresses in dependencies must be unique.'):

This comment has been minimized.

@illicitonion

illicitonion Mar 7, 2018

Contributor

Again, having both //:a and other:b were good...

This comment has been minimized.

@stuhood

stuhood Mar 8, 2018

Member

This one was just truncation of the message. Fixed.

mode: The mode to write to the file in - over-write by default.
"""
if self._graph_helper is not None:
self._graph_helper.scheduler.invalidate_files(list(self._collect_invalid_for(relpath)))

This comment has been minimized.

@illicitonion

illicitonion Mar 7, 2018

Contributor

The invalidation should probably happen after the write, rather than before. Otherwise if we end up with e.g. background threads consuming these, they may consume the events too quickly.

This comment has been minimized.

@stuhood

stuhood Mar 8, 2018

Member

Not expecting any threads here, but sure... can't hurt.

@stuhood stuhood force-pushed the twitter:stuhood/engine-in-base-test branch from 5a72ecc to 8b3d459 Mar 8, 2018

@illicitonion
Copy link
Contributor

illicitonion left a comment

Works for me :) Thanks!

@benjyw
Copy link
Contributor

benjyw left a comment

Huzzah for the name change! But it makes it hard to see what the salient changes are to BaseTest/TestBase itself. Any pointers on what to look at?

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Mar 8, 2018

OK, diffed the two files directly, so LGTM as far as I can tell.

@benjyw

benjyw approved these changes Mar 8, 2018

@kwlzn

kwlzn approved these changes Mar 9, 2018

yield f
prev = f
f = os.path.dirname(f)
yield ''

This comment has been minimized.

@kwlzn

kwlzn Mar 9, 2018

Member

was there an actual bug with the old impl here? that seemed cleaner and more readable to me.

This comment has been minimized.

@stuhood

stuhood Mar 10, 2018

Member

Yea: infinite loop for . as an input.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Mar 10, 2018

Don't forget to apply the relevant diff from #5575 to the new TestBase.

@stuhood stuhood force-pushed the twitter:stuhood/engine-in-base-test branch from 3f6720e to 92912ab Mar 11, 2018

stuhood added some commits Mar 11, 2018

@stuhood stuhood merged commit b4e8aa2 into pantsbuild:master Mar 12, 2018

1 check passed

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

@stuhood stuhood deleted the twitter:stuhood/engine-in-base-test branch Mar 12, 2018

stuhood added a commit to twitter/pants that referenced this pull request Mar 13, 2018

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