Switch the new PytestRun task to use junitxml output. #4403

Merged
merged 4 commits into from Mar 30, 2017

Conversation

Projects
None yet
2 participants
@benjyw
Contributor

benjyw commented Mar 29, 2017

This addresses #3837

This change also created a unit test for the new task, based
on the old task's tests with appropriate changes. Somehow this
test wasn't created when the task was.

A few small tweaks were necessary to get the tests to pass.

This change also moves the existing pytest run integration test
into the directory containing all the other new-task tests, so
we don't forget it when we delete the old tasks and their tests.
That integration test has been running the new task since we
switched that to be the default in the repo anyway.

Note that the old task no longer has an integration test (and
hasn't since we made the switch to the new one in our repo)
but that seems fine since it's going to be deprecated and
removed soon.

Switch the new PytestRun task to use junitxml output.
This addresses #3837

This change also created a unit test for the new task, based
on the old task's tests with appropriate changes.  Somehow this
test wasn't created when the task was.

A few small tweaks were necessary to get the tests to pass.

This change also moves the existing pytest run integration test
into the directory containing all the other new-task tests, so
we don't forget it when we delete the old tasks and their tests.
That integration test has been running the new task since we
switched that to be the default in the repo anyway.

Note that the old task no longer has an integration test (and
hasn't since we made the switch to the new one in our repo)
but that seems fine since it's going to be deprecated and
removed soon.
@kwlzn

kwlzn approved these changes Mar 30, 2017

lgtm! thanks Benjy!

@@ -80,6 +80,8 @@ def register_options(cls, register):
'create a new chroot, which will be much slower, but more correct, as the '
'isolation verifies that all dependencies are correctly declared.')
register('--junit-xml-dir', metavar='<DIR>',
+ removal_version='1.5.0.dev0',
+ removal_hint='Unused. We always use JUnit XML results files.',

This comment has been minimized.

@kwlzn

kwlzn Mar 30, 2017

Member

seems like this would still be generally useful for emitting a copy of the junit xml results to a workdir-external path? from a quick look, it appears we actually use this for that purpose in our CI.

@kwlzn

kwlzn Mar 30, 2017

Member

seems like this would still be generally useful for emitting a copy of the junit xml results to a workdir-external path? from a quick look, it appears we actually use this for that purpose in our CI.

This comment has been minimized.

@benjyw

benjyw Mar 30, 2017

Contributor

Does "we" here mean "Twitter"?

@benjyw

benjyw Mar 30, 2017

Contributor

Does "we" here mean "Twitter"?

This comment has been minimized.

@benjyw

benjyw Mar 30, 2017

Contributor

Changed the task so it copies the xml file into this dir, if specified.

@benjyw

benjyw Mar 30, 2017

Contributor

Changed the task so it copies the xml file into this dir, if specified.

This comment has been minimized.

@kwlzn

kwlzn Mar 30, 2017

Member

Does "we" here mean "Twitter"?

yep. in CI, we do the equivalent of a ./pants changed --dependees=transitive | xargs -L1 -P800 ./pants test across a distributed worker pool - so this provides a convenient way to dump and gather the output for summarization.

thanks!

@kwlzn

kwlzn Mar 30, 2017

Member

Does "we" here mean "Twitter"?

yep. in CI, we do the equivalent of a ./pants changed --dependees=transitive | xargs -L1 -P800 ./pants test across a distributed worker pool - so this provides a convenient way to dump and gather the output for summarization.

thanks!

+ # Now grab the classnames from the xml file.
+ xml = XmlParser.from_file(junitxml)
+ classname_and_names = [(testcase.getAttribute('classname'), testcase.getAttribute('name'))
+ for testcase in xml.parsed.getElementsByTagName('testcase')]

This comment has been minimized.

@kwlzn

kwlzn Mar 30, 2017

Member

how about making this a generator comp ((...)) vs list comp ([...])?

@kwlzn

kwlzn Mar 30, 2017

Member

how about making this a generator comp ((...)) vs list comp ([...])?

This comment has been minimized.

@benjyw

benjyw Mar 30, 2017

Contributor

Done.

@benjyw

benjyw Mar 30, 2017

Contributor

Done.

+ methodname=testcase.getAttribute('name'))
+ target = test_registry.get_owning_target(test)
+ #if not target:
+ # raise ValueError('No target found for test: {}'.format(test.render_test_spec()))

This comment has been minimized.

@kwlzn

kwlzn Mar 30, 2017

Member

dead code?

@kwlzn

kwlzn Mar 30, 2017

Member

dead code?

This comment has been minimized.

@benjyw

benjyw Mar 30, 2017

Contributor

Ah, I needed to comment that out because it was obscuring some other problem. But in general I think this check should be present. Have restored it, will see if the CI passes.

@benjyw

benjyw Mar 30, 2017

Contributor

Ah, I needed to comment that out because it was obscuring some other problem. But in general I think this check should be present. Have restored it, will see if the CI passes.

@benjyw benjyw merged commit 6d86256 into pantsbuild:master Mar 30, 2017

1 check passed

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

@benjyw benjyw deleted the benjyw:pytest_junitxml branch Mar 30, 2017

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

Switch the new PytestRun task to use junitxml output. (#4403)
This addresses pantsbuild#3837

This change also created a unit test for the new task, based
on the old task's tests with appropriate changes.  Somehow this
test wasn't created when the task was.

A few small tweaks were necessary to get the tests to pass.

This change also moves the existing pytest run integration test
into the directory containing all the other new-task tests, so
we don't forget it when we delete the old tasks and their tests.
That integration test has been running the new task since we
switched that to be the default in the repo anyway.

Note that the old task no longer has an integration test (and
hasn't since we made the switch to the new one in our repo)
but that seems fine since it's going to be deprecated and
removed soon.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment