Wrong test changes reported in every PR

[HeikoKlare]

For several days, the post of the test results bot in every PR lists changed (removed/added) tests (see bot report example below). The report looks obivously broken as the actual test results do not indicate an issue, there is not such a high number of changed tests in any of the affected PRs, and log extract posted by the bot shows strange output. Still, that report is quite important for every PR as it tells us if we have accidentally removed any tests. But no one seems to care, as PRs are being merged without any note of the report.

I cannot properly trace the occurrence of the broken reports back to a PR merged in the Platform repository.
One of the last PRs that had proper bot report was this:

• Simplify ColorManager and use modern MT idioms #2642

The following PR could have been the last with or without proper bot report. It was merged before CI results (and bot report) were available, which is why CI and bot report results are missing:

• Fix assertion in testBug546710_ConsoleCreationRaceCondition(TestInfo) #2641

That PR did affect console tests though, which are now listed in the strange output in the bot report. However, the report in #2642 states that it was compared against the commit of #2641 without showing the wrong bot report result.

The first PR that that contained the broken bot report and was merged with it was this:

• Fix progress overshoot >100% in Synchronize refresh jobs #2646

So the issue may not have been introduced by any change in this repository but by a change somewhere else.

I have also not found the log extracts posted by the bot in either of the CI logs (GHA or Jenkins), so I am currently not sure where they are extracted from.

Bot Report Example

From #2643:

This pull request removes 387 and adds 276 tests. Note that renamed tests count towards both.

, nl=[Ljava.lang.String;@10bad903]
, nl=[Ljava.lang.String;@138eac9f]
, nl=[Ljava.lang.String;@14e1214a]
, nl=[Ljava.lang.String;@1a6c8679]
, nl=[Ljava.lang.String;@1c93d91c]
, nl=[Ljava.lang.String;@1d112dbf]
, nl=[Ljava.lang.String;@1fc74fd3]
, nl=[Ljava.lang.String;@221e21ee]
, nl=[Ljava.lang.String;@2350ee6d]
, nl=[Ljava.lang.String;@240e7fb]
…

org.eclipse.debug.tests.console.ConsoleOutputLineTruncateTest ‑ test(Parameters)[10] Parameters[limit=2, chunks=5, repeat=1, input=====
====, output=== ...
== ...
== ...
== ...
== ...
== ...
, nl=[Ljava.lang.String;@1927ff2d]
org.eclipse.debug.tests.console.ConsoleOutputLineTruncateTest ‑ test(Parameters)[10] Parameters[limit=2, chunks=5, repeat=1, input=====
====, output=== ...
== ...
== ...
== ...
== ...
== ...
, nl=[Ljava.lang.String;@5379985a]
org.eclipse.debug.tests.console.ConsoleOutputLineTruncateTest ‑ test(Parameters)[10] Parameters[limit=2, chunks=5, repeat=1, input=====
====, output=== ...
== ...
== ...
== ...
== ...
== ...
, nl=[Ljava.lang.String;@5c66a6ad]
org.eclipse.debug.tests.console.ConsoleOutputLineTruncateTest ‑ test(Parameters)[11] Parameters[limit=2, chunks=1, repeat=1, input==========, output=== ...
, nl=[Ljava.lang.String;@4d32ede]
org.eclipse.debug.tests.console.ConsoleOutputLineTruncateTest ‑ test(Parameters)[11] Parameters[limit=2, chunks=1, repeat=1, input==========, output=== ...
, nl=[Ljava.lang.String;@651e5038]
org.eclipse.debug.tests.console.ConsoleOutputLineTruncateTest ‑ test(Parameters)[11] Parameters[limit=2, chunks=1, repeat=1, input==========, output=== ...
, nl=[Ljava.lang.String;@794b1fac]
org.eclipse.debug.tests.console.ConsoleOutputLineTruncateTest ‑ test(Parameters)[12] Parameters[limit=2, chunks=1, repeat=1, input========
==, output=== ...
==, nl=[Ljava.lang.String;@61b4d99a]
org.eclipse.debug.tests.console.ConsoleOutputLineTruncateTest ‑ test(Parameters)[12] Parameters[limit=2, chunks=1, repeat=1, input========
==, output=== ...
==, nl=[Ljava.lang.String;@6a37bd6b]
org.eclipse.debug.tests.console.ConsoleOutputLineTruncateTest ‑ test(Parameters)[12] Parameters[limit=2, chunks=1, repeat=1, input========
==, output=== ...
==, nl=[Ljava.lang.String;@743b4b76]
org.eclipse.debug.tests.console.ConsoleOutputLineTruncateTest ‑ test(Parameters)[13] Parameters[limit=3, chunks=1, repeat=1, input==========, output==== ...
, nl=[Ljava.lang.String;@1e791302]
…

[laeubi]

You need to look at this action: https://github.com/eclipse-platform/eclipse.platform/actions/workflows/unit-tests.yml
One issue might be that somehow the test-artifacts names are not unique what will overwrite one with the other randomly.

[HeikoKlare]

The action uses the test results pushed by the CI action (i.e., the Jenkins build). What I just fount is that the logs posted by the bot are exactly the same in every PR. And I do not find those log extracts in the according Jenkins builds. As the posted logs contain some hashes/memory addresses (which are the same in every PR), it seems like the actual test results are not properly updated but the same test results are used for every report.

[laeubi]

The action uses the test results pushed by the CI action (i.e., the Jenkins build)

This would be very surprising, Jenkins has nothing to do with GH actions.

And I do not find those log extracts in the according Jenkins builds

Because they are not related to Jenkins at all.

There are two kinds of uploads, one is the "Event File" what records the context: https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/45cf785c41846fbc497cacee8f59657a2d9c5077/.github/workflows/mavenBuild.yml#L31-L39

and one is the test results: https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/45cf785c41846fbc497cacee8f59657a2d9c5077/.github/workflows/mavenBuild.yml#L89-L97

Both are then picked up by this action here

https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/45cf785c41846fbc497cacee8f59657a2d9c5077/.github/workflows/publishTestResults.yml#L37-L47
https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/45cf785c41846fbc497cacee8f59657a2d9c5077/.github/workflows/publishTestResults.yml#L54

[HeikoKlare]

Sorry, I meant to write GHA, not Jenkins. The information posted by the bot is extracted from the surefire reports that are contained in the archives uploaded by the action publishing test results. So the reference reports taken for comparison seem to be wrong/outdated.

[laeubi]

For me the output about removed test look suspicious ... more like some random test output than to actual tests.... so maybe something is broken in the upload itself or the format changed in some unexspected way or our tests emit something that confuses the action.

[HeikoKlare]

Yes, that looks strange. It seems to be an extract of the test case name. They look like this

<testcase name="test(Parameters)[43] Parameters[limit=3, chunks=1, repeat=1, input=ab😀c, output=ab&#10;😀c, nl=[Ljava.lang.String;@19db879c]" classname="org.eclipse.debug.tests.console.ConsoleOutputLineWrapTest" time="0.003"/>

See that there is the , nl=[L... part that is present in the removed tests log. Seeing this, my first guess is that the smileys (or some other special characters) in there break something. The according tests have been added 5 days ago with:

• Add hard wrap limit option to console #2494

@trancexpress fyi

That PR was even merged after the ones I mentioned in the original post. I missed that one as GitHub by default sorts the closed PRs by the date they were opened and not by the date they were merged and that one was open for quite a while.

[trancexpress]

We have added tests with such symbols a while ago: #2466

E.g.: https://github.com/eclipse-platform/eclipse.platform/pull/2466/changes#diff-2f537dd358966335edeeb4d71c0252d2228545b5015fe86b8721680e67ad4dc9R753

I do see such e-mails from a few weeks ago:

This pull request removes 1 and adds 500 tests. Note that renamed tests count towards both.

For: eclipse-platform/eclipse.platform.ui#3896 (comment)

If you suspect the symbols, we can try the unicode codepoint, I'm not sure if that will work (for the new tests) but the test already has such from 2019.

The tests pass with this:

diff --git a/debug/org.eclipse.debug.tests/src/org/eclipse/debug/tests/console/IOConsoleTests.java b/debug/org.eclipse.debug.tests/src/org/eclipse/debug/tests/console/IOConsoleTests.java
index d9393986ee..8c764d7686 100644
--- a/debug/org.eclipse.debug.tests/src/org/eclipse/debug/tests/console/IOConsoleTests.java
+++ b/debug/org.eclipse.debug.tests/src/org/eclipse/debug/tests/console/IOConsoleTests.java
@@ -81,6 +81,11 @@ import org.junit.jupiter.api.extension.ExtendWith;
 @ExtendWith(DebugTestExtension.class)
 public class IOConsoleTests {
 
+       /**
+        * Used to test that surrogate pairs are not split by console limits.
+        */
+       private static final String SMILEY = String.valueOf(Character.toChars(128512));
+
        /**
         * The console view used for the running test. Required to obtain access to
         * consoles {@link StyledText} widget to simulate user input.
@@ -751,13 +756,13 @@ public class IOConsoleTests {
                final IOConsoleTestUtil c = getTestUtil("Test trim newline");
                try (IOConsoleOutputStream out = c.getDefaultOutputStream()) {
                        c.writeFast("first\n");
-                       c.writeFast("1😀2😀3\n", out);
+                       c.writeFast("1" + SMILEY + "2" + SMILEY + "3\n", out);
                        c.write("last\n");
                        c.verifyContentByLine("first", 0).verifyContentByLine("last", -2);
                        assertTrue(c.getDocument().getNumberOfLines() > 2, "Document not filled.");
                        c.getConsole().setWaterMarks(8, 16);
                        c.waitForScheduledJobs();
-                       c.verifyContentByOffset("😀3\nlas", 0);
+                       c.verifyContentByOffset(SMILEY + "3\nlas", 0);
                        closeConsole(c);
                }
        }
@@ -810,12 +815,12 @@ public class IOConsoleTests {
                try (IOConsoleOutputStream out = c.getDefaultOutputStream()) {
                        c.getConsole().setLimitLineLength(false, 8);
                        c.writeFast("first\n");
-                       c.writeFast("01234😀😀😀\n", out);
+                       c.writeFast("01234" + SMILEY + SMILEY + SMILEY + "\n", out);
                        c.write("last\n");
                        c.verifyContentByLine("first", 0).verifyContentByLine("last", -2);
                        assertTrue(c.getDocument().getNumberOfLines() > 2, "Document not filled.");
                        c.waitForScheduledJobs();
-                       c.verifyContent("first\n01234😀 ...\nlast\n");
+                       c.verifyContent("first\n01234" + SMILEY + " ...\nlast\n");
                        closeConsole(c);
                }
        }
@@ -870,12 +875,12 @@ public class IOConsoleTests {
                        String nl = c.getConsole().getDocument().getLegalLineDelimiters()[0];
                        c.getConsole().setLimitLineLength(true, 8);
                        c.writeFast("first\n");
-                       c.writeFast("0123456😀😀\n", out);
+                       c.writeFast("0123456" + SMILEY + SMILEY + "\n", out);
                        c.write("last\n");
                        c.verifyContentByLine("first", 0).verifyContentByLine("last", -2);
                        assertTrue(c.getDocument().getNumberOfLines() > 2, "Document not filled.");
                        c.waitForScheduledJobs();
-                       c.verifyContent("first\n0123456" + nl + "😀😀\nlast\n");
+                       c.verifyContent("first\n0123456" + nl + SMILEY + SMILEY + "\nlast\n");
                        closeConsole(c);
                }
        }

Let me know if I should set a PR with this. If such symbols are suspect, there is no harm to replace them.

[iloveeclipse]

We recently had JUnit update in the platform: eclipse-platform/eclipse.platform.releng.aggregator#3798

Can it be, the JUnit guys broke something once again?

[HeikoKlare]

We have added tests with such symbols a while ago: #2466

E.g.: https://github.com/eclipse-platform/eclipse.platform/pull/2466/changes#diff-2f537dd358966335edeeb4d71c0252d2228545b5015fe86b8721680e67ad4dc9R753

The reason for the broken test reports being symbols like emojis was just a guess. But note that it's about the symbols being part of the test name. In the referenced PR, such symbols were used inside test execution but not in the test name itself, if I am not mistaken.
Whether or not the emojis are causing the issue, the output at least references exactly the ConsoleOutputLineTruncateTest/ConsoleOutputLineWrapTest that has recently been added, so I am quite confident that something in the test name is causing the issues.
Edit: Further investingating the logs, it may rather be the line breaks than the emojis in the test names causing the issue.

My first try to fix those tests would be defining a simplified DisplayName so that the test name does not contain any special symbols.

I do see such e-mails from a few weeks ago:
This pull request removes 1 and adds 500 tests. Note that renamed tests count towards both.

For: eclipse-platform/eclipse.platform.ui#3896 (comment)

Such reports are intended in case test number actually change (like it's the case in the mentioned PR). We currently get those reports even though there are not actual changes in test number. And that seems to be caused by the mentioned, recently added tests as those are exactly the tests that are named in the logs for removed/added tests.

We recently had JUnit update in the platform: eclipse-platform/eclipse.platform.releng.aggregator#3798

Can it be, the JUnit guys broke something once again?

Of course something like that may also cause such issues. But since the JUnit change to Platform was several days before the test reports broke, since no other repository seems to have such issues and since the logs of the broken test report point to exactly the recently added ConsoleOutputLineTruncateTest/ConsoleOutputLineWrapTest, I suspect them to be causing the issue first.

[trancexpress]

Such reports are intended in case test number actually change (like it's the case in the mentioned PR).

I'm not seeing test changes in eclipse-platform/eclipse.platform.ui#3896, or its force-pushed commits, but as you say.

I've set #2651, please review.

[HeikoKlare]

I'm not seeing test changes in eclipse-platform/eclipse.platform.ui#3896, or its force-pushed commits, but as you say.

The test was changed to be executed as repeated test (500 times): eclipse-platform/eclipse.platform.ui@9216956
That produced the test report that you have mentioned.

Then the PR was updated so that the test is again only executed once. That triggered an update of the test report, which is why the final bot comment does not report any changes in the number of executed tests: eclipse-platform/eclipse.platform.ui#3896 (comment)

The test report that you have mentioned is only visible in the history of the bot comment (you may have found it in your mailbox?), as it has been overwritten after the test change had been reverted.