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

move the declarative task stuff out of the python backend testing #7279

Merged

Conversation

Projects
None yet
2 participants
@cosmicexplorer
Copy link
Contributor

commented Feb 22, 2019

Problem

We want to do #7128, which involves adding some more C/C++ unit testing. We would love to use some of the declarative task specification stuff we've added to the python_dist() testing to do that.

Solution

  • Move the generic logic out of BuildLocalPythonDistributionsTestBase into DeclarativeTaskTestMixin in task_test_base.py.

Result

A new interface for task unit testing (which can be extended to more than just v1 tasks) is available to other testing.

Show resolved Hide resolved tests/python/pants_test/engine/scheduler_test_base.py Outdated
Show resolved Hide resolved tests/python/pants_test/engine/scheduler_test_base.py Outdated
Show resolved Hide resolved tests/python/pants_test/engine/scheduler_test_base.py Outdated
single_product = all_products[0]
return single_product

def _create_task(self, task_type, context):

This comment has been minimized.

Copy link
@stuhood

stuhood Feb 23, 2019

Member

TaskTestBase should handle this I think.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 1, 2019

Author Contributor

Added a docstring -- this is used specifically to hydrate the run_before_task_types and run_after_task_types in self.invoke_tasks().

cosmicexplorer added some commits Mar 1, 2019

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

Should have addressed the comments above, and finally managed to get a green CI run!

@stuhood
Copy link
Member

left a comment

Thanks.

raise NotImplementedError('dist_specs must be implemented!')

@classproperty
def run_before_task_types(cls):

This comment has been minimized.

Copy link
@stuhood

stuhood Mar 2, 2019

Member

The run_before and run_after tasks look very important for correct usage of this class, but it's not clear to me what they would do (without reading the implementation of this class).

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 3, 2019

Author Contributor

Added lots more documentation (and removed the previous docstrings)!

"""Tasks to run after local dists are built, similar to `run_before_task_types`."""
return []

def populate_target_dict(self):

This comment has been minimized.

Copy link
@stuhood

stuhood Mar 2, 2019

Member

This helper method doesn't seem like it is specific to this class. You could probably pass in the dist_specs as a map, and return the created targets (which you could then pass to invoke_tasks). That would allow the method to move up to TestBase, and make it useful in more places.

The rest of the Task manipulation stuff could probably stay here.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 3, 2019

Author Contributor

Done!

@stuhood

stuhood approved these changes Mar 3, 2019

Copy link
Member

left a comment

Thanks!

The values of `target_map` should themselves be a flat dict, which contains keyword arguments
fed into `self.make_target()`, along with a few special keys. Special keys are:
- 'key' (required -- used to index into the returned dict).

This comment has been minimized.

Copy link
@stuhood

stuhood Mar 3, 2019

Member

Could this be an address, or address string?

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 3, 2019

Author Contributor

It could default to that, actually -- the idea was to decouple accessing the target from within testing from its specific address so you could use a test class-specific shorthand.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 3, 2019

Author Contributor

It now defaults to the target address string!

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:make-ctypes-tests-unit-tests branch from 187d7ff to c6a4031 Mar 3, 2019

@cosmicexplorer cosmicexplorer merged commit 29bb3d0 into pantsbuild:master Mar 3, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.