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

Start migrating away from SchedulerTestBase #5929

Merged
merged 4 commits into from Jul 26, 2018

Conversation

Projects
None yet
2 participants
@illicitonion
Copy link
Contributor

illicitonion commented Jun 7, 2018

Commits individually reviewable.

More to come.

@illicitonion illicitonion requested a review from stuhood Jun 7, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks Daniel. I think AddressMapperTest should continue to use the lower level API though.

def scheduler_execute_expecting_one_result(self, product, subject):
request = self.scheduler.execution_request([product], [subject])
result = self.scheduler.execute(request)

This comment has been minimized.

@stuhood

stuhood Jun 7, 2018

Member

This method can be simplified to:

res, = self.scheduler.product_request(product, [subject])
return res

... or just inlined, I think.

symbol_table = TargetTable()
address_mapper = AddressMapper(parser=JsonParser(symbol_table),
build_patterns=('*.BUILD.json',))
TestBase.setUp(self)

This comment has been minimized.

@stuhood

stuhood Jun 7, 2018

Member

I think the recommendation is to use super(...).setUp() because it only needs to refer to the name of this class, rather than needing edits when the inheritance hierarchy changes.

@@ -145,21 +145,33 @@ def unhydrated_structs(build_file_addresses):
yield UnhydratedStructs(uhs)


class AddressMapperTest(unittest.TestCase, SchedulerTestBase):
class AddressMapperTest(TestBase, unittest.TestCase):

This comment has been minimized.

@stuhood

stuhood Jun 7, 2018

Member

This class might be a bit of a special case: it doesn't use the BuildFileAliases, or anything else from the legacy package. It's intentionally testing other parsers in an isolated environment. So it might be one of the few cases that should actually use a scheduler directly.

# Eagerly free file handles, threads, connections, etc, held by the scheduler. In theory,
# dropping the scheduler is equivalent, but it's easy for references to the scheduler to leak.
self._scheduler.pre_fork()

def reset_scheduler(self):

This comment has been minimized.

@stuhood

stuhood Jun 7, 2018

Member

I don't think we should do this in order to accommodate AddressMapperTest and other tests that are intentionally testing at a lower level.

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/schedulertestbase branch 3 times, most recently from 04ea823 to 5b6e7fe Jun 8, 2018

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Jun 8, 2018

Reverted the test_mapper changes, removed scheduler_execute_expecting_one_result, added more detail to some exceptions

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/schedulertestbase branch from 5b6e7fe to aecfb3f Jul 26, 2018

@illicitonion illicitonion merged commit 5f9adf2 into pantsbuild:master Jul 26, 2018

1 check passed

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

@illicitonion illicitonion deleted the twitter:dwagnerhall/schedulertestbase branch Jul 26, 2018

CMLivingston pushed a commit to CMLivingston/pants that referenced this pull request Aug 27, 2018

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