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

Conversation

XJ114514
Copy link

@XJ114514 XJ114514 commented Sep 23, 2024

Overview

Current PR can successfully build and generate junit-platform-console-standalone-1.12.0-SNAPSHOT.jar containing diff outputs for two different Strings to the console.

Changes made in this draft PR:

  • add diff function to junit-platform-console/.../tasks/ConsoleTestExecutor.java (output of console)
  • add dependency to libs.versions.toml and junit-platform-console.gradle.kts of junit-platform-console
  • add changed that will be made to release-notes-5.12.0-M1.adoc

Tasks that need to finish

  • add package documentation to junit-platform-console/.../module-info.java (causing testing failure because the auto test cases can not find the diff package)
  • add auto test cases for diff output in ConsoleTestExecutorTests.java

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


Definition of Done


continue from #3397
Issue: #3139

XuJ321 and others added 13 commits August 31, 2024 18:25
Tasks completed:
1. Locate ConsoleLauncher output
2. Check if actual and expected of AssertionFailedError are both type of CharSequence
To do list:
1. Add diff function dependency
2. Using diff to generate the output desired

Issue: junit-team#3139
Tasks completed:
1. Locate ConsoleLauncher output
2. Check if actual and expected of AssertionFailedError are both type of CharSequence
3. Add diff function dependency
4. Using diff to generate the output desired

Issue: junit-team#3139
delete one line of extra code
Fixed different profile issue.

Tasks completed:
1. Locate ConsoleLauncher output
2. Check if actual and expected of AssertionFailedError are both type of CharSequence
3. Add diff function dependency
4. Using diff to generate the output desired

ToDo:
1.Automatic test case for diff output

Issue: junit-team#3139
Add external diff module  to module documentation
For unknown reason, the test cases can not find the external module while the gradle can build normally.

Issue: junit-team#3139
@marcphilipp
Copy link
Member

@XJ114514 Could you please change this PR to continue where #3397 left off? We can then tackle the problems that remain one by one.

@marcphilipp marcphilipp marked this pull request as draft September 23, 2024 13:30
@sbrannen sbrannen changed the title Issue: junit-team#3139 Draft PR Output diff of expected and actual values in ConsoleLauncher Sep 23, 2024
@XJ114514
Copy link
Author

@XJ114514 Could you please change this PR to continue where #3397 left off? We can then tackle the problems that remain one by one.

I apologize for the trouble. But you already superseded and closed #3397. Is there anything I need to do?

@marcphilipp
Copy link
Member

I closed it because the original author didn't respond anymore. But if you're interested you could create a new branch in your repo based on their work and make the changes that were requested in #3397. Please let me know whether you want to proceed that way.

@XJ114514
Copy link
Author

I prefer staying in this PR. The primary goal is to output diff info in the console, create test cases, and successfully merge them.

The extra changes requested in #3397:

Shadowing the original external function
Using theme to highlight the diff info
spreading diff info to all the Junit API output

should come later as it would be easier once we have one successful example.

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.

The extra changes requested in #3397:

Shadowing the original external function
Using theme to highlight the diff info
spreading diff info to all the Junit API output

should come later as it would be easier once we have one successful example.

We usually only merge complete PRs that leave main ready to be released. But I can help with the shadowing.

gradle/libs.versions.toml Outdated Show resolved Hide resolved
@@ -180,11 +186,39 @@ private Optional<TestExecutionListener> createXmlWritingListener(PrintWriter out
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())) {
//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?

1. Add diffPrinter class
2. replace diff in consoleTestExecutor with diffPrinter - will be deleted evetually
3. mark the location of adding diff in flatPrinter and verboseTreePrinter.

Issue: junit-team#3139
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.

I think the diffs need to reference the failures, i.e. something like this (where (2) and (3) reference failures from the list above):

Failures (3):
  (1) JUnit Jupiter:AssertionsDemo:groupedAssertions()
    MethodSource [className = 'example.AssertionsDemo', methodName = 'groupedAssertions', methodParameterTypes = '']
    => org.opentest4j.AssertionFailedError: expected: <2> but was: <3>
       org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
       org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
       org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
       org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150)
       org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:145)
       org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:531)
       example.AssertionsDemo.groupedAssertions(AssertionsDemo.java:52)
  (2) JUnit Jupiter:AssertionsDemo:dependentAssertions()
    MethodSource [className = 'example.AssertionsDemo', methodName = 'dependentAssertions', methodParameterTypes = '']
    => org.opentest4j.AssertionFailedError: expected: <a
b
c> but was: <b
c
d>
       org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
       org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
       org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
       org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
       org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:177)
       org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1145)
       example.AssertionsDemo.dependentAssertions(AssertionsDemo.java:63)
  (3) JUnit Jupiter:AssertionsDemo:standardAssertions()
    MethodSource [className = 'example.AssertionsDemo', methodName = 'standardAssertions', methodParameterTypes = '']
    => org.opentest4j.AssertionFailedError: expected: <fooooooo> but was: <booooooy>
       org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
       org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
       org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
       org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
       org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:177)
       org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1145)
       example.AssertionsDemo.standardAssertions(AssertionsDemo.java:41)

Diffs (Markdown):
  (2) JUnit Jupiter:AssertionsDemo:groupedAssertions()
   | expected | actual |
   | -------- | ------ |
   | ~~a~~ | b |
   | b | c |
   | c | **d** |
  (3) JUnit Jupiter:AssertionsDemo:standardAssertions()
   | expected | actual |
   | -------- | ------ |
   | ~~f~~oooooo~~o~~ | **b**oooooo**y** |

WDYT?

static void printDiff(PrintWriter out, String expected, String actual) {
DiffRowGenerator generator = DiffRowGenerator.create().showInlineDiffs(true).inlineDiffByWord(true).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.

XJ114514 and others added 2 commits October 8, 2024 22:12
@XJ114514 XJ114514 closed this Oct 9, 2024
@XJ114514 XJ114514 reopened this Oct 9, 2024
Only if one word has space, show inlineDiffByWord.
@XJ114514
Copy link
Author

I think the diffs need to reference the failures, i.e. something like this (where (2) and (3) reference failures from the list above):

Failures (3):
  (1) JUnit Jupiter:AssertionsDemo:groupedAssertions()
    MethodSource [className = 'example.AssertionsDemo', methodName = 'groupedAssertions', methodParameterTypes = '']
    => org.opentest4j.AssertionFailedError: expected: <2> but was: <3>
       org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
       org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
       org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
       org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150)
       org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:145)
       org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:531)
       example.AssertionsDemo.groupedAssertions(AssertionsDemo.java:52)
  (2) JUnit Jupiter:AssertionsDemo:dependentAssertions()
    MethodSource [className = 'example.AssertionsDemo', methodName = 'dependentAssertions', methodParameterTypes = '']
    => org.opentest4j.AssertionFailedError: expected: <a
b
c> but was: <b
c
d>
       org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
       org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
       org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
       org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
       org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:177)
       org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1145)
       example.AssertionsDemo.dependentAssertions(AssertionsDemo.java:63)
  (3) JUnit Jupiter:AssertionsDemo:standardAssertions()
    MethodSource [className = 'example.AssertionsDemo', methodName = 'standardAssertions', methodParameterTypes = '']
    => org.opentest4j.AssertionFailedError: expected: <fooooooo> but was: <booooooy>
       org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
       org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
       org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
       org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
       org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:177)
       org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1145)
       example.AssertionsDemo.standardAssertions(AssertionsDemo.java:41)

Diffs (Markdown):
  (2) JUnit Jupiter:AssertionsDemo:groupedAssertions()
   | expected | actual |
   | -------- | ------ |
   | ~~a~~ | b |
   | b | c |
   | c | **d** |
  (3) JUnit Jupiter:AssertionsDemo:standardAssertions()
   | expected | actual |
   | -------- | ------ |
   | ~~f~~oooooo~~o~~ | **b**oooooo**y** |

WDYT?

I can only get the test's display name (the actual test method name). In MutuableTestExecutionSummary, the rest info is from the parent(s) of the testIdentifier. I don't see a simple way to do so in other classes. WDYT?

image

@marcphilipp
Copy link
Member

If we list them separately, we should definitely output the same info as done by MutableTestExecutionSummary. I think all we'd need would be a TestExecutionListener that keeps hold of the TestPlan passed to testPlanExecutionStarted which we could use to access information about parent descriptors. Do you think you could give that a try?

1. Add a `testPlan` getter to `platform/launcher/listeners/SummaryGeneratingListener.java`.

2. DiffPrinter now needs testPlan to display the testMethod info.
Issue: junit-team#3139
@XJ114514
Copy link
Author

If we list them separately, we should definitely output the same info as done by MutableTestExecutionSummary. I think all we'd need would be a TestExecutionListener that keeps hold of the TestPlan passed to testPlanExecutionStarted which we could use to access information about parent descriptors. Do you think you could give that a try?

The printFoundTestsSummary() was not called / MutableTestExecutionSummary was not created in ConsoleTestExecutor in summary mode. What's worse, the testPlan was never visible to ConsoleTestExecutor
Therefore, I add a testPlan getter to platform/launcher/listeners/SummaryGeneratingListener.java. Will that create any security issue?

Update DiffPrinter to contain testPlan as an attribute and display the testMethod info when printing. Preview is below.

image

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.

The printFoundTestsSummary() was not called / MutableTestExecutionSummary was not created in ConsoleTestExecutor in summary mode. What's worse, the testPlan was never visible to ConsoleTestExecutor
Therefore, I add a testPlan getter to platform/launcher/listeners/SummaryGeneratingListener.java. Will that create any security issue?

Rather than adding it to SummaryGeneratingListener, I'd register another (local) listener for this purpose.

Update DiffPrinter to contain testPlan as an attribute and display the testMethod info when printing. Preview is below.

Looks like we're getting closer. We're still missing the (2) etc. references, though.

@XJ114514
Copy link
Author

XJ114514 commented Oct 21, 2024

The printFoundTestsSummary() was not called / MutableTestExecutionSummary was not created in ConsoleTestExecutor in summary mode. What's worse, the testPlan was never visible to ConsoleTestExecutor
Therefore, I add a testPlan getter to platform/launcher/listeners/SummaryGeneratingListener.java. Will that create any security issue?

Rather than adding it to SummaryGeneratingListener, I'd register another (local) listener for this purpose.

Update DiffPrinter to contain testPlan as an attribute and display the testMethod info when printing. Preview is below.

Looks like we're getting closer. We're still missing the (2) etc. references, though.

I don't know how to create a local listener. Could you step in and make one? Sorry for my ignorance.
In CI/CD checks, many tests were ignored and were treated as failed tests in the report after I made the change, do you know what is wrong?
My console output has no (2) reference before each test failure.
image

@marcphilipp
Copy link
Member

I don't know how to create a local listener. Could you step in and make one? Sorry for my ignorance.

For some reason, GitHub didn't let me push a commit to your PR so I opened a PR: XJ114514#1

In CI/CD checks, many tests were ignored and were treated as failed tests in the report after I made the change, do you know what is wrong?

We use Predictive Test Selection so the prior build that ignored them probably didn't select the ones that failed in the most recent build.

My console output has no (2) reference before each test failure.

Sorry for not being clear. I think we should add those there as well.

@XJ114514
Copy link
Author

XJ114514 commented Oct 25, 2024

For some reason, GitHub didn't let me push a commit to your PR so I opened a PR: XJ114514#1

I manually combined the change you made to this PR, but the testPlan from testPlanCapturer is null when using. Can you check about it, sorry I can not offer help because I don't understand how the TestExecutionListener works.

Sorry for not being clear. I think we should add those there as well.

I found the testIdentifier.getUniqueId() has the sequential numbers at the end and it works for my small test cases.
Would you use this standalone generated from current PR to run special cases to see if it will work? You can also generate the standalone by going to the branch and run gradle.
getUniqueId() code:
image
preview from my two simple test methods
image
image

@marcphilipp marcphilipp self-requested a review November 4, 2024 09:54
@XJ114514
Copy link
Author

XJ114514 commented Nov 5, 2024

I tried to run the ./gradle test since the Predictive Test Selection will skip the tests.
The tests failed because we inserted the diff(markdown) section and ins number reference to the front of the test title - the screenshot below.

  1. Should I change the tests or move the diff(markdown) section to the bottom and remove the number reference in SummaryGeneratingListener?
  2. The testPlan listener you provided is not workinould we just use the testPlan getter in SummaryGeneratingListener?
    image
    image

@marcphilipp
Copy link
Member

marcphilipp commented Nov 6, 2024

@XJ114514 Let's put this PR on hold for now. I'm currently working on a new HTML report format that would make it much easier to display a diff.

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.

ConsoleLauncher should output diff of expected and actual String values
4 participants