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

Output diff of expected and actual values in ConsoleLauncher #4017

Draft
wants to merge 27 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
e0d2849
Issue: #3139 part 1
XuJ321 Aug 31, 2024
bc1b464
Issue: #3139 part 2
XuJ321 Aug 31, 2024
2fa9a8e
Issue: #3139 part 2 update
XuJ321 Aug 31, 2024
50552b4
Issue: junit-team#3139 part 2 update
XJ114514 Sep 1, 2024
e8d11bd
Merge branch 'junit-team:main' into main
XJ114514 Sep 13, 2024
7b4088d
Fix format issue
XJ114514 Sep 14, 2024
bb29695
fix build documentation issue
XJ114514 Sep 14, 2024
2c27b53
trying to fix the build error
XJ114514 Sep 14, 2024
af16685
Try to Fix Dependency issue
XJ114514 Sep 14, 2024
3d73d1e
Issue: junit-team#3139 Add external diff module to module documentation
XJ114514 Sep 14, 2024
c8a7b95
Merge branch 'junit-team:main' into main
XJ114514 Sep 23, 2024
53d884c
Remove module-info.java documentation for testing
XJ114514 Sep 23, 2024
26bcaa4
Run spotless and do final testing before draft PR
XJ114514 Sep 23, 2024
3248752
Update libs.versions.toml
XJ114514 Sep 25, 2024
04f789a
Adding diffPrinter class
XJ114514 Oct 7, 2024
983e090
Update junit-platform-console/src/main/java/org/junit/platform/consol…
XJ114514 Oct 9, 2024
eef49f1
Update junit-platform-console/src/main/java/org/junit/platform/consol…
XJ114514 Oct 9, 2024
bcd0608
Update junit-platform-console/src/main/java/org/junit/platform/consol…
XJ114514 Oct 9, 2024
cc69bfb
adjust the diffPrinter output format
XJ114514 Oct 9, 2024
705b4cf
Update the rule of inlineDiffByWord
XJ114514 Oct 9, 2024
2383f3e
update the format of printing diff
XJ114514 Oct 12, 2024
82f723a
Issue: junit-team#3139 Update to display each test method info
XJ114514 Oct 19, 2024
536e9c7
Print `Diffs (Markdown)` only once when needed
XJ114514 Oct 21, 2024
f44f929
apply spotless
XJ114514 Oct 21, 2024
eef975b
Combine PR made by Marc from XJ114514#1
XJ114514 Oct 25, 2024
de00ade
Revert "Combine PR made by Marc from XJ114514#1"
XJ114514 Oct 25, 2024
a3f8f25
Issue: junit-team#3139 Add number reference to each test
XJ114514 Oct 25, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
*Date of Release:* ❓

*Scope:* ❓
* `ConsoleLauncher` output shows extra diff message for failed assertions on two `CharSequence` objects

For a complete list of all _closed_ issues and pull requests for this release, consult the
link:{junit5-repo}+/milestone/75?closed=1+[5.12.0-M1] milestone page in the
Expand Down
1 change: 1 addition & 0 deletions gradle/libs.versions.toml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ jfrunit = { module = "org.moditect.jfrunit:jfrunit-core", version = "1.0.0.Alpha
jimfs = { module = "com.google.jimfs:jimfs", version = "1.3.0" }
jmh-core = { module = "org.openjdk.jmh:jmh-core", version.ref = "jmh" }
jmh-generator-annprocess = { module = "org.openjdk.jmh:jmh-generator-annprocess", version.ref = "jmh" }
java-diff-utils = { module = "io.github.java-diff-utils:java-diff-utils", version = "4.12" }
joox = { module = "org.jooq:joox", version = "2.0.1" }
jte = { module = "gg.jte:jte", version = "3.1.12" }
junit4 = { module = "junit:junit", version = { require = "[4.12,)", prefer = "4.13.2" } }
Expand Down
6 changes: 5 additions & 1 deletion junit-platform-console/junit-platform-console.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ dependencies {

compileOnly(libs.openTestReporting.events)

implementation(libs.java.diff.utils)

shadowed(libs.picocli)

osgiVerification(projects.junitJupiterEngine)
Expand All @@ -28,7 +30,9 @@ tasks {
"--add-modules", "org.opentest4j.reporting.events",
"--add-reads", "${project.projects.junitPlatformReporting.dependencyProject.javaModuleName}=org.opentest4j.reporting.events",
"--add-modules", "info.picocli",
"--add-reads", "${javaModuleName}=info.picocli"
"--add-reads", "${javaModuleName}=info.picocli",
"--add-modules", "io.github.javadiffutils",
"--add-reads", "${javaModuleName}=io.github.javadiffutils"
))
}
shadowJar {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
import org.junit.platform.launcher.listeners.SummaryGeneratingListener;
import org.junit.platform.launcher.listeners.TestExecutionSummary;
import org.junit.platform.reporting.legacy.xml.LegacyXmlReportGeneratingListener;
import org.opentest4j.AssertionFailedError;
import org.opentest4j.ValueWrapper;

/**
* @since 1.0
Expand All @@ -47,6 +49,8 @@ public class ConsoleTestExecutor {
private final TestConsoleOutputOptions outputOptions;
private final Supplier<Launcher> launcherSupplier;

private TestPlan testPlanListeners;

public ConsoleTestExecutor(TestDiscoveryOptions discoveryOptions, TestConsoleOutputOptions outputOptions) {
this(discoveryOptions, outputOptions, LauncherFactory::create);
}
Expand Down Expand Up @@ -77,7 +81,6 @@ private void discoverTests(PrintWriter out) {

LauncherDiscoveryRequest discoveryRequest = new DiscoveryRequestCreator().toDiscoveryRequest(discoveryOptions);
TestPlan testPlan = launcher.discover(discoveryRequest);

commandLineTestPrinter.ifPresent(printer -> printer.listTests(testPlan));
if (outputOptions.getDetails() != Details.NONE) {
printFoundTestsSummary(out, testPlan);
Expand All @@ -97,12 +100,12 @@ private static void printFoundTestsSummary(PrintWriter out, TestPlan testPlan) {
private TestExecutionSummary executeTests(PrintWriter out, Optional<Path> reportsDir) {
Launcher launcher = launcherSupplier.get();
SummaryGeneratingListener summaryListener = registerListeners(out, reportsDir, launcher);

LauncherDiscoveryRequest discoveryRequest = new DiscoveryRequestCreator().toDiscoveryRequest(discoveryOptions);
launcher.execute(discoveryRequest);

TestExecutionSummary summary = summaryListener.getSummary();
if (summary.getTotalFailureCount() > 0 || outputOptions.getDetails() != Details.NONE) {
//get testPlan from summaryListener
testPlanListeners = summaryListener.getTestPlan();
printSummary(summary, out);
}

Expand Down Expand Up @@ -181,10 +184,30 @@ private void printSummary(TestExecutionSummary summary, PrintWriter out) {
// Otherwise the failures have already been printed in detail
if (EnumSet.of(Details.NONE, Details.SUMMARY, Details.TREE).contains(outputOptions.getDetails())) {
summary.printFailuresTo(out);
//adding diff code here
Copy link
Member

Choose a reason for hiding this comment

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

This should be done in place where we print the exceptions (as it is in #3397). Otherwise, it would be difficult to find the test that belongs to the diff in case of multiple failing tests.

Copy link
Author

Choose a reason for hiding this comment

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

image
image

Which part do you wish to change? The first print happens in DefaultLaucher and the second happens in MutableTestExecutionSummary. Both are in junit-platform-laucher module. Changes will affect all Junit API output I believe?
I can try to work on it over the weekend if you wish.
At the same time, can you help to find why the package documentation test case fails (Test cases can not find diff package once I add it to the module-info.java for documentation)

Copy link
Member

Choose a reason for hiding this comment

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

We also need to make sure that the diff is printed if a details mode other than Details.NONE, Details.SUMMARY, or Details.TREE is used. We should be able to extract the diff-printing code to a separate class and call it from MutableTestExecutionSummary, and the other listeners (FlatPrintingListener etc.).

Copy link
Author

@XJ114514 XJ114514 Sep 27, 2024

Choose a reason for hiding this comment

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

We also need to make sure that the diff is printed if a details mode other than Details.NONE, Details.SUMMARY, or Details.TREE is used. We should be able to extract the diff-printing code to a separate class and call it from MutableTestExecutionSummary, and the other listeners (FlatPrintingListener etc.).

The solutions I can think of:

  1. add diff output function to junit-platform-commons...util.StringUtils
  2. Use the diff output function in FlatPrintingListener, TreePrintingListener, VerboseTreePrintingListener and MutableTestExecutionSummary
    I will start to work on it if the solution looks right for you

Copy link
Member

Choose a reason for hiding this comment

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

  1. add diff output function to junit-platform-commons...util.StringUtils

I think it should stay in junit-platform-console. Please create a separate DiffPrinter class or similar.

Copy link
Author

Choose a reason for hiding this comment

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

  1. DiffPrinter class has been added.
  2. Marked the location of adding diff to FlatPrintingListener and VerboseTreePrintingListener
  3. Will the diff info be generated after the stack trace of the exception?

out.printf("%nDiffs (Markdown):%n");
summary.getFailures().forEach(failure -> {
//get AssertionFailedError
if (failure.getException() instanceof AssertionFailedError) {
AssertionFailedError assertionFailedError = (AssertionFailedError) failure.getException();
ValueWrapper expected = assertionFailedError.getExpected();
ValueWrapper actual = assertionFailedError.getActual();
//apply diff function
if (isCharSequence(expected) && isCharSequence(actual)) {
new DiffPrinter(testPlanListeners).printDiff(out, expected.getStringRepresentation(),
actual.getStringRepresentation(), failure.getTestIdentifier());
}

}
});
}
summary.printTo(out);
}

private boolean isCharSequence(ValueWrapper value) {
return value != null && CharSequence.class.isAssignableFrom(value.getType());
}

@FunctionalInterface
public interface Factory {
ConsoleTestExecutor create(TestDiscoveryOptions discoveryOptions, TestConsoleOutputOptions outputOptions);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/*
* Copyright 2015-2024 the original author or authors.
*
* All rights reserved. This program and the accompanying materials are
* made available under the terms of the Eclipse Public License v2.0 which
* accompanies this distribution and is available at
*
* https://www.eclipse.org/legal/epl-v20.html
*/

package org.junit.platform.console.tasks;

import static java.lang.String.join;

import java.io.PrintWriter;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

import com.github.difflib.text.DiffRow;
import com.github.difflib.text.DiffRowGenerator;

import org.junit.platform.launcher.TestIdentifier;
import org.junit.platform.launcher.TestPlan;

/**
* class provide access to printDiff function
*/
class DiffPrinter {
private final TestPlan testPlan;

public DiffPrinter(TestPlan testPlan) {
this.testPlan = testPlan;
}

//print the difference of two print to out
void printDiff(PrintWriter out, String expected, String actual, TestIdentifier testIdentifier) {
out.printf(" %s:", describeTest(testIdentifier));
boolean inlineDiffByWordFlag = false;
if (expected.contains(" ") || actual.contains(" ")) {
inlineDiffByWordFlag = true;
}
DiffRowGenerator generator = DiffRowGenerator.create().showInlineDiffs(true).inlineDiffByWord(
inlineDiffByWordFlag).oldTag(f -> "~~").newTag(f -> "**").build();
List<DiffRow> rows = generator.generateDiffRows(Arrays.asList(expected), Arrays.asList(actual));
Copy link
Member

Choose a reason for hiding this comment

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

Don't we have to split the expected and actual Strings into lines here?

Copy link
Author

Choose a reason for hiding this comment

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

What the difference is if we split them into lines?

Copy link
Member

Choose a reason for hiding this comment

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

I wasn't sure. I haven't used this library before and I was just curious why it expects lists of Strings. Do you know the reason?

Copy link
Author

Choose a reason for hiding this comment

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

I don't know the details but all their methods use List<String> as input.

out.println();
out.println(" | expected | actual |");
out.println(" | -------- | ------ |");
for (DiffRow row : rows) {
XJ114514 marked this conversation as resolved.
Show resolved Hide resolved
out.printf(" | %s | %s |", row.getOldLine(), row.getNewLine());
out.println();
}
}

private String describeTest(TestIdentifier testIdentifier) {
List<String> descriptionParts = new ArrayList<>();
collectTestDescription(testIdentifier, descriptionParts);
return join(":", descriptionParts);
}

private void collectTestDescription(TestIdentifier identifier, List<String> descriptionParts) {
descriptionParts.add(0, identifier.getDisplayName());
testPlan.getParent(identifier).ifPresent(parent -> collectTestDescription(parent, descriptionParts));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ private void printlnTestDescriptor(Style style, String message, TestIdentifier t

private void printlnException(Style style, Throwable throwable) {
printlnMessage(style, "Exception", ExceptionUtils.readStackTrace(throwable));
//add diff here
}

private void printlnMessage(Style style, String message, String detail) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ public void executionStarted(TestIdentifier testIdentifier) {
@Override
public void executionFinished(TestIdentifier testIdentifier, TestExecutionResult testExecutionResult) {
testExecutionResult.getThrowable().ifPresent(t -> printDetail(Style.FAILED, "caught", readStackTrace(t)));
//add diff here
if (testIdentifier.isContainer()) {
Long creationMillis = frames.pop();
printVerticals(theme.end());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,6 @@
requires org.junit.platform.launcher;
requires org.junit.platform.reporting;


provides java.util.spi.ToolProvider with org.junit.platform.console.ConsoleLauncherToolProvider;
}
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,13 @@ public TestExecutionSummary getSummary() {
return this.summary;
}

/**
* Get the testPlan of this listener.
*/
public TestPlan getTestPlan() {
return testPlan;
}

@Override
public void testPlanExecutionStarted(TestPlan testPlan) {
this.testPlan = testPlan;
Expand Down
Loading