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

Update junit-runner to 1.0.24 and use junit-runner-annotations 0.0.21 in tests #5721

Merged
merged 1 commit into from Apr 19, 2018

Conversation

Projects
None yet
2 participants
@cheister
Copy link
Contributor

cheister commented Apr 18, 2018

Problem

Updating master to use the latest version of the junit-runner

Solution

Update the version of junit-runner to 1.0.24.

Main changes included in the update are

#5667 Add explicit JAXB dependencies in the junit-runner so it works in Java 9+ without --add-modules=java.xml.bind
#5660 [junit-runner] cache localhost lookups to ease OSX/JDK DNS issues
#5614 Memoize org.scalatest.Suite class loading

Result

Tests are passing. The new junit-runner jar includes jaxb jars which required some changes to the shading excludes of the junit-runner shaded jar.

@cheister cheister requested review from baroquebobcat and stuhood Apr 18, 2018

.format(junit_xml_path, cause))
self._junit_xml_path = junit_xml_path
.format(xml_path, cause))
self._xml_path = xml_path

This comment has been minimized.

@cheister

cheister Apr 18, 2018

Contributor

This change was needed so this ParseError has the same properties as the ParseError definied in src/python/pants/task/testrunner_task_mixin.py. I'm not sure if we would rather consolidate these into one ParseError class or not? Either of these errors can be thrown when parsing an xml file and the src/python/pants/backend/jvm/tasks/junit_run.py file handles them with the following method, which expects xml_path.

    def parse_error_handler(parse_error):
      # Just log and move on since the result is only used to characterize failures, and raising
      # an error here would just distract from the underlying test failures.
      self.context.log.error('Error parsing test result file {path}: {cause}'
                             .format(path=parse_error.xml_path, cause=parse_error.cause))

This comment has been minimized.

@baroquebobcat

baroquebobcat Apr 18, 2018

Contributor

Oh, so this fixes a bug with junit_run where it didn't work for both exceptions? Seems reasonable. If they're both used to parse junit xml files, I think consolidating them makes sense, but maybe not part of this version bump.

Also, is this change related to the version bump, or could it be broken out along with the consolidation?

This comment has been minimized.

@cheister

cheister Apr 19, 2018

Contributor

Yes. Since either ParseError could be thrown, this fixes the logger when the junit_xml_parser's ParseError is thrown.

It's not strictly related to the version bump, but it was exposed if you don't include the extra shading exclusions for the junit-runner.

This comment has been minimized.

@baroquebobcat

baroquebobcat Apr 19, 2018

Contributor

Oh. I see. That's fine then.

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

looks good. I've got a question about the ParseError change before I'll approve though.

.format(junit_xml_path, cause))
self._junit_xml_path = junit_xml_path
.format(xml_path, cause))
self._xml_path = xml_path

This comment has been minimized.

@baroquebobcat

baroquebobcat Apr 18, 2018

Contributor

Oh, so this fixes a bug with junit_run where it didn't work for both exceptions? Seems reasonable. If they're both used to parse junit xml files, I think consolidating them makes sense, but maybe not part of this version bump.

Also, is this change related to the version bump, or could it be broken out along with the consolidation?

@cheister cheister merged commit 30c6110 into pantsbuild:master Apr 19, 2018

1 check passed

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

@cheister cheister deleted the cheister:update-junit-runner-0.24 branch Apr 19, 2018

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