Move the pytest-related runtime requirement specs into a subsystem. #4071

Merged
merged 2 commits into from Nov 20, 2016

Conversation

Projects
None yet
3 participants
@benjyw
Contributor

benjyw commented Nov 18, 2016

No description provided.

@benjyw

This comment has been minimized.

Show comment
Hide comment
@benjyw

benjyw Nov 18, 2016

Contributor

My original pass actually removed the requirement injection from pytest_run.py, and made all python_tests targets implicitly depend on those requirements (as we do for JUnit). But that isn't really correct - these deps provide the test runner (as a tool), but the test may not even know or care that it's being run by pytest. So I left the requirement injection where it is.

Contributor

benjyw commented Nov 18, 2016

My original pass actually removed the requirement injection from pytest_run.py, and made all python_tests targets implicitly depend on those requirements (as we do for JUnit). But that isn't really correct - these deps provide the test runner (as a tool), but the test may not even know or care that it's being run by pytest. So I left the requirement injection where it is.

@jsirois

This comment has been minimized.

Show comment
Hide comment
@jsirois

jsirois Nov 20, 2016

Member

My original pass actually removed the requirement injection from pytest_run.py, and made all python_tests targets implicitly depend on those requirements (as we do for JUnit). But that isn't really correct - these deps provide the test runner (as a tool), but the test may not even know or care that it's being run by pytest. So I left the requirement injection where it is.

+1 to to this

Member

jsirois commented Nov 20, 2016

My original pass actually removed the requirement injection from pytest_run.py, and made all python_tests targets implicitly depend on those requirements (as we do for JUnit). But that isn't really correct - these deps provide the test runner (as a tool), but the test may not even know or care that it's being run by pytest. So I left the requirement injection where it is.

+1 to to this

@jsirois

All small stuff noted.

+# Copyright 2016 Pants project contributors (see CONTRIBUTORS.md).
+# Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+

This comment has been minimized.

@jsirois

jsirois Nov 20, 2016

Member

Kill extra blank line.

@jsirois

jsirois Nov 20, 2016

Member

Kill extra blank line.

This comment has been minimized.

@benjyw

benjyw Nov 20, 2016

Contributor

Fixed.

@benjyw

benjyw Nov 20, 2016

Contributor

Fixed.

+ @classmethod
+ def register_options(cls, register):
+ super(PyTest, cls).register_options(register)
+ register('--pytest-requirements', default='pytest>=2.6,<2.7',

This comment has been minimized.

@jsirois

jsirois Nov 20, 2016

Member

Why the re-namespacing here?, ie: it seems s/pytest-// is in order for all these --pytest- prefixed options. The lone remaining long-form --pytest-unittest2-requirements vs --pytest-requirements seems like it would be clear enough to me coupled with the help strings.

@jsirois

jsirois Nov 20, 2016

Member

Why the re-namespacing here?, ie: it seems s/pytest-// is in order for all these --pytest- prefixed options. The lone remaining long-form --pytest-unittest2-requirements vs --pytest-requirements seems like it would be clear enough to me coupled with the help strings.

This comment has been minimized.

@benjyw

benjyw Nov 20, 2016

Contributor

So weird, I thought I had indeed done that! I remember thinking it would be nicer, but I guess didn't end up following through. Done. Also made all these options advanced, which they should have been to begin with.

@benjyw

benjyw Nov 20, 2016

Contributor

So weird, I thought I had indeed done that! I remember thinking it would be nicer, but I guess didn't end up following through. Done. Also made all these options advanced, which they should have been to begin with.

+ help='Requirements string for the unittest2 library, which some python versions '
+ 'may need.')
+
+ def get_requirement_strings(self):

This comment has been minimized.

@jsirois

jsirois Nov 20, 2016

Member

A doc-comment would be great here.

@jsirois

jsirois Nov 20, 2016

Member

A doc-comment would be great here.

This comment has been minimized.

@benjyw

benjyw Nov 20, 2016

Contributor

Done.

@benjyw

benjyw Nov 20, 2016

Contributor

Done.

@benjyw benjyw merged commit c07e6b3 into pantsbuild:master Nov 20, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@benjyw benjyw deleted the benjyw:pytest_deps branch Nov 20, 2016

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