Skip to content

Conversation

@mmerdes
Copy link
Contributor

@mmerdes mmerdes commented Jan 21, 2019

Overview

This PR adds overloaded assertEquals() and assertNotEqualsMethods() to reestablish
compatibility with Groovy's dynamic method dispatch.

Fixes #1710.


I hereby agree to the terms of the JUnit Contributor License Agreement.


Definition of Done

@ghost ghost assigned mmerdes Jan 21, 2019
@ghost ghost added the status: reviewing label Jan 21, 2019
@codecov
Copy link

codecov bot commented Jan 21, 2019

Codecov Report

Merging #1741 into master will increase coverage by 3.79%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1741      +/-   ##
============================================
+ Coverage     88.47%   92.27%   +3.79%     
- Complexity     3902     4678     +776     
============================================
  Files           350      351       +1     
  Lines          9574    11018    +1444     
  Branches        777      905     +128     
============================================
+ Hits           8471    10167    +1696     
+ Misses          901      656     -245     
+ Partials        202      195       -7
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/org/junit/jupiter/api/Assertions.java 98.2% <100%> (+0.35%) 482 <85> (+249) ⬆️
...rg/junit/jupiter/api/support/io/TempDirectory.java 88.88% <0%> (-1.53%) 32% <0%> (+16%)
...jupiter/engine/execution/ExtensionValuesStore.java 87.14% <0%> (-1.43%) 21% <0%> (ø)
...gine/descriptor/JupiterEngineExtensionContext.java 76.47% <0%> (-1.31%) 11% <0%> (+5%)
.../java/org/junit/platform/runner/JUnitPlatform.java 97.38% <0%> (-0.54%) 71% <0%> (+24%)
...auncher/listeners/MutableTestExecutionSummary.java 94.23% <0%> (-0.22%) 48% <0%> (+12%)
...ain/java/org/junit/platform/launcher/TestPlan.java 97.46% <0%> (-0.21%) 29% <0%> (+12%)
...nit/platform/engine/support/hierarchical/Node.java 82% <0%> (-0.15%) 16% <0%> (+6%)
...it/platform/commons/support/AnnotationSupport.java 100% <0%> (ø) 26% <0%> (+19%) ⬆️
... and 46 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1cabcc...e1090b2. Read the comment docs.

@codecov
Copy link

codecov bot commented Jan 21, 2019

Codecov Report

Merging #1741 into master will increase coverage by 1.11%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1741      +/-   ##
============================================
+ Coverage     88.47%   89.59%   +1.11%     
- Complexity     3902     4443     +541     
============================================
  Files           350      350              
  Lines          9574    10833    +1259     
  Branches        777      889     +112     
============================================
+ Hits           8471     9706    +1235     
- Misses          901      925      +24     
  Partials        202      202
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/org/junit/jupiter/api/Assertions.java 98.21% <100%> (+0.36%) 474 <71> (+241) ⬆️
...gine/descriptor/JupiterEngineExtensionContext.java 76.47% <0%> (-1.31%) 11% <0%> (+5%)
.../java/org/junit/platform/runner/JUnitPlatform.java 97.38% <0%> (-0.54%) 71% <0%> (+24%)
...auncher/listeners/MutableTestExecutionSummary.java 94.23% <0%> (-0.22%) 48% <0%> (+12%)
...ain/java/org/junit/platform/launcher/TestPlan.java 97.46% <0%> (-0.21%) 29% <0%> (+12%)
...nit/platform/engine/support/hierarchical/Node.java 82% <0%> (-0.15%) 16% <0%> (+6%)
...it/platform/commons/support/AnnotationSupport.java 100% <0%> (ø) 13% <0%> (+6%) ⬆️
...piter/engine/descriptor/ClassExtensionContext.java 100% <0%> (ø) 17% <0%> (+7%) ⬆️
...java/org/junit/jupiter/api/AssertDoesNotThrow.java 100% <0%> (ø) 18% <0%> (+7%) ⬆️
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1cabcc...9c0ee0b. Read the comment docs.

Copy link
Member

@marcphilipp marcphilipp left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

I think we should add smoke tests for Java and Koltin, e.g. assertEquals() with all combinations of int and Integer.

@sbrannen Didn't we add @API and @since when adding the additional overloads in 5.3?

@marcphilipp marcphilipp added this to the 5.4 RC1 milestone Jan 21, 2019
@sbrannen
Copy link
Member

sbrannen commented Jan 22, 2019

@sbrannen Didn't we add @API and @since when adding the additional overloads in 5.3?

We added them for the overloads in 5.4, for example:

	/**
	 * <em>Assert</em> that {@code expected} and {@code actual} are not equal.
	 *
	 * <p>If necessary, the failure message will be retrieved lazily from the
	 * supplied {@code messageSupplier}.
	 *
	 * @since 5.4
	 */
	@API(status = STABLE, since = "5.4")
	public static void assertNotEquals(byte unexpected, Byte actual, Supplier<String> messageSupplier) {
		AssertNotEquals.assertNotEquals((Byte) unexpected, actual, messageSupplier);
	}

@sbrannen
Copy link
Member

@sbrannen Didn't we add @API and @since when adding the additional overloads in 5.3?

So, yes, we need those there, too.

@sbrannen
Copy link
Member

I think we should add smoke tests for Java and Koltin, e.g. assertEquals() with all combinations of int and Integer.

That's definitely not a bad idea, just more busy work (unless we auto-generate those as you've suggested elsewhere).

Copy link
Member

@sbrannen sbrannen left a comment

Choose a reason for hiding this comment

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

At a cursory glance, it LGTM as well.

As @marcphilipp mentioned please add @API and @since to the new overloaded methods.

@sbrannen sbrannen merged commit c8a49a0 into master Jan 24, 2019
@ghost ghost removed the status: reviewing label Jan 24, 2019
@marcphilipp marcphilipp deleted the issues/1710-groovy-assertions-fixes branch April 14, 2019 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants