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

Upgrade junit to 4.13 #51787

Closed
wants to merge 3 commits into from
Closed

Upgrade junit to 4.13 #51787

wants to merge 3 commits into from

Conversation

racevedoo
Copy link
Contributor

There should be no breaking changes. Release notes at: https://github.com/junit-team/junit4/blob/master/doc/ReleaseNotes4.13.md

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Build)

@pugnascotia
Copy link
Contributor

@elasticmachine ok to test

@pugnascotia
Copy link
Contributor

@racevedoo CI has reported test failes, and there are merge conflicts now too.

@rjernst
Copy link
Member

rjernst commented Feb 15, 2020

@elasticmachine update branch

@elasticmachine
Copy link
Collaborator

merge conflict between base and head

@rjernst
Copy link
Member

rjernst commented Feb 18, 2020

There are 2 failures that need to be investigated/worked on:

  • buildSrc's testKit tests have an embedded ES build, which for some reason is running dependencyLicenses on tests deps, and has a sha1 for the junit jar within the test source
  • There are dozens of uses of an existing assertThrows method from ElasticsearchAssertions. This method is statically imported, and is now being masked by the static method from Assert which we pick up because LuceneTestCase extends from Assert. These methods should possibly be converted to the new assertThrows, but give their signatures, for now we should either rename or qualify with the classname.

jaymode added a commit to jaymode/elasticsearch that referenced this pull request Feb 20, 2020
This commit renames ElasticsearchAssertions#assertThrows to
assertThrown to avoid a naming clash with JUnit 4.13+ and static
imports of these methods. Additionally, these methods have been updated
to make use of expectThrows internally to avoid duplicating the logic
there.

Relates elastic#51787
@rjernst
Copy link
Member

rjernst commented Feb 21, 2020

We discussed this PR in the core/infra weekly and while we appreciate the attempt, we don't think it is worth merging at this time. The main reason is that we would like to stay in sync with RandomizedRunner and Lucene as to which junit version we are using. However, we would like to be prepared for that eventuality, so Jay has opened a PR (#52582) to rename the offending method name collisions.

@rjernst rjernst closed this Feb 21, 2020
jaymode added a commit that referenced this pull request Feb 21, 2020
This commit renames ElasticsearchAssertions#assertThrows to
assertRequestBuilderThrows and assertFutureThrows to avoid a
naming clash with JUnit 4.13+ and static imports of these methods.
Additionally, these methods have been updated to make use of
expectThrows internally to avoid duplicating the logic there.

Relates #51787
jaymode added a commit to jaymode/elasticsearch that referenced this pull request Feb 21, 2020
This commit renames ElasticsearchAssertions#assertThrows to
assertRequestBuilderThrows and assertFutureThrows to avoid a
naming clash with JUnit 4.13+ and static imports of these methods.
Additionally, these methods have been updated to make use of
expectThrows internally to avoid duplicating the logic there.

Relates elastic#51787
jaymode added a commit that referenced this pull request Feb 21, 2020
This commit renames ElasticsearchAssertions#assertThrows to
assertRequestBuilderThrows and assertFutureThrows to avoid a
naming clash with JUnit 4.13+ and static imports of these methods.
Additionally, these methods have been updated to make use of
expectThrows internally to avoid duplicating the logic there.

Relates #51787
Backport of #52582
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants