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

JTH 2.46 #3872

Merged
merged 1 commit into from
Feb 8, 2019
Merged

JTH 2.46 #3872

merged 1 commit into from
Feb 8, 2019

Conversation

thomasgl-orange
Copy link
Contributor

@thomasgl-orange thomasgl-orange commented Jan 30, 2019

See jenkinsci/jenkins-test-harness#89.

Bump JTH from 2.42 to 2.46, mainly to test Jenkins itself with a space in temp dir path.

Proposed changelog entries

Submitter checklist

  • JIRA issue is well described => no Jira issue, this is just a followup for [RFC] space char in tmp dirs to catch quoting issues jenkins-test-harness#89
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change).
  • Appropriate autotests or explanation to why this change has no tests => no additionnal test, just checking that existing tests are fine
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

(several reviewers are already there)

@Wadeck
Copy link
Contributor

Wadeck commented Jan 30, 2019

@thomasgl-orange the most problematic feature is the space added by default in the path that failed ~48 tests in my previous PR.

More information / analysis in #3618 about the correction / troubles encountered.

Thank you for coming here to help the bump :) Don't hesitate to ping me if you need help.

Note also that there is a change in the way the JavaScript events are triggered that need to be adapted in the tests.

@thomasgl-orange
Copy link
Contributor Author

@Wadeck Thanks for the heads-up, I had completely forgotten to search for other similar PR before starting this one. I see you've dealt with everything related to the htmlunit incompatibilities already, nice!

So, I will rewrite this PR based on yours (currently Wadeck/jenkins@297104a4, with JTH 2.42 and all tests being OK), and I will only do the 2.42=>2.46 bump part, fixing the space-in-path mess I have introduced with jenkinsci/jenkins-test-harness#89 :)

From what I see of the failed tests so far, it all comes down to xalan being in the classpath of the tests. The thing is, to workaround a xalan bug, I had to maven-shade it in jenkinsci/jenkins-test-harness-htmlunit@da3e4d2 (2.31-2, used by JTH 2.46), to stop it from auto-registering as the JAXP TransformerFactory provider. It was enough to avoid the issue with JTH 2.46 in plugins (which usually don't depend on xalan themselves), but here, in jenkins itself, we have test-pom which has its own explicit dependency on xalan (6cf6776), because the xalan API is used in some test-jdk8 tests (several ones under the jenkins.security.security218.ysoserial package). The dependency makes sense (better being explicit, since it's actually used), but still, what I will propose here is to drop it, and rely on the shaded classes from JTH-htmlunit, because it should work good enough.

I'm push-forcing that now, will see how tests go on this PR (they're still running on my laptop).

If this is not acceptable, some alternatives I can think of are:

  • forcing javax.xml.transform.TransformerFactory to the default JDK-8 implementation in test and test-jdk8 (either via a property in the Surefire config, or via a META-INF file)
  • re-packaging xalan somewhere else, and using that artifact as a dependency from test-pom (could actually be from test-jdk8 only), instead of the upstream version

@jglick
Copy link
Member

jglick commented Jan 30, 2019

we have test-pom which has its own explicit dependency on xalan (6cf6776), because the xalan API is used in some test-jdk8 tests (several ones under the jenkins.security.security218.ysoserial package)

#3838 deletes that stuff. It is obsolete.

@Wadeck
Copy link
Contributor

Wadeck commented Jan 31, 2019

@thomasgl-orange when I see the simple commit that solve the issue, I am banging my head on the wall :D Please look at the #3618 about proposal for the next step with this PR.

@oleg-nenashev oleg-nenashev added the on-hold This pull request depends on another event/release, and it cannot be merged right now label Feb 1, 2019
@oleg-nenashev
Copy link
Member

@thomasgl-orange Thanks for the pull request! You might need to rebase your patches once #3618 is merged, because the commits were squashed there

jglick added a commit to jglick/jenkins that referenced this pull request Feb 5, 2019
@batmat
Copy link
Member

batmat commented Feb 7, 2019

@oleg-nenashev why did you put this on-hold BTW, just so we know if/when we can mark it ready-for-merge and proceed. Thanks

@oleg-nenashev
Copy link
Member

@batmat It was on hold, because it depends on #3618 . I will process it once that guy is merged (tomorrow?)

@oleg-nenashev
Copy link
Member

@thomasgl-orange #3618 has been merged. Could you please rebase your changes so that we have a clean PR?

@oleg-nenashev oleg-nenashev removed the on-hold This pull request depends on another event/release, and it cannot be merged right now label Feb 8, 2019
@batmat batmat changed the base branch from master to data-api February 8, 2019 06:18
@batmat batmat changed the base branch from data-api to master February 8, 2019 06:18
@batmat
Copy link
Member

batmat commented Feb 8, 2019

Update done (trick is to change target branch to another and back)

@batmat batmat requested review from Wadeck and jglick February 8, 2019 07:43
@batmat
Copy link
Member

batmat commented Feb 8, 2019

@thomasgl-orange can you now please restore the PR template, check the right checkboxes, and mainly write a changelog?

I would like to merge this soon.

Thanks!

@batmat
Copy link
Member

batmat commented Feb 8, 2019

@thomasgl-orange also,I suppose we can now consider this non WIP anymore right?

Copy link
Contributor

@Wadeck Wadeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's perfect, thank very much Thomas

@thomasgl-orange thomasgl-orange changed the title WIP: JTH 2.46 JTH 2.46 Feb 8, 2019
@thomasgl-orange
Copy link
Contributor Author

@batmat: OK, I've edited the PR title and description. And no, it's no more WIP.

@oleg-nenashev oleg-nenashev merged commit ba1a27d into jenkinsci:master Feb 8, 2019
@batmat batmat removed the request for review from jglick February 8, 2019 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants