Skip to content

Put two failing Gradle test to ignore list - #3298

Closed
lkishalmi wants to merge 1 commit into
apache:masterfrom
lkishalmi:ignore-ever-failing-gradle-tests
Closed

Put two failing Gradle test to ignore list#3298
lkishalmi wants to merge 1 commit into
apache:masterfrom
lkishalmi:ignore-ever-failing-gradle-tests

Conversation

@lkishalmi

Copy link
Copy Markdown
Contributor

These two tests seems to be failing, at least locally.

VerifyGradleProjectTemplatesTest.java has a missing template engine. That may need a better test classpath.

testCompilePreTrusted() it probably would be nice that one time this would pass. though right now we do nothing with non-opened project, even when it has cached information.

So this one is for making Travis happy.

@lkishalmi lkishalmi added this to the 12.6 milestone Nov 3, 2021
@JaroslavTulach

Copy link
Copy Markdown

I haven't seen (continuous) failures of the VerifyGradleProjectTemplatesTest on Travis and it works on my computer:

netbeans$ ant build
netbeans$ ant -f java/gradle.java test-single -Dtest.includes=**/VerifyGradleProjectTemplatesTest*
BUILD SUCCESSFUL

VerifyGradleProjectTemplatesTest.java has a missing template engine. That may need a better test classpath.

The test is using NbModuleSuite with all modules enabled. Possibly you need to build the IDE first.

@JaroslavTulach JaroslavTulach left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for letting me know before removing my test. I placed it into Gradle modules to give the maintainers an early warning that a Gradle related functionality is broken. If that is causing problems (because not fully built IDE?), then I'd rather move test into commit validation elsewhere than disable it altogether.

@lkishalmi

Copy link
Copy Markdown
Contributor Author

That test fails for me, though I have to admit I only built the IDE with java cluster as target (as far as I know that should add the ide cluster as well which contains the freemarker engine).

@JaroslavTulach

Copy link
Copy Markdown

That test fails for me, though I have to admit I only built the IDE with java cluster as target
(as far as I know that should add the ide cluster as well which contains the freemarker engine).

$ git log | head -n1
commit b4f2bb2af5ea0ac17c22c802f17011d6aea09b05
$ ant build -Dcluster.config=java
$ ant -f java/gradle.java test-single -Dtest.includes=**/VerifyGradleProjectTemplatesTest*
BUILD SUCCESSFUL

e.g. it works for me as well on my jdk1.8.0_212 on Ubuntu. I suggest to report a bug with steps to reproduce rather than adding @Ignore to my test.

@sdedic

sdedic commented Nov 3, 2021

Copy link
Copy Markdown
Member

For example:

�[0K$ hide-logs.sh ant $OPTS -f java/gradle.java test
travis_time:end:049e38ac:start=1635865291766791993,finish=1635865480430581959,duration=188663789966,event=script
�[0K�[32;1mThe command "hide-logs.sh ant $OPTS -f java/gradle.java test" exited with 0.�[0m

See e.g. randomly chosen https://app.travis-ci.com/github/apache/netbeans/jobs/546271812

If there are steps to reproduce the failure +- reliably from the clean state, please file a bug. The test is not meaningless: it was created to check that if a project has been trusted, its files can be opened (e.g. Goto Type) with the correct classpath - that's done without UI-opening project in the IDE.

@neilcsmith-net neilcsmith-net added the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label Nov 3, 2021
@neilcsmith-net

Copy link
Copy Markdown
Member

Syncing for RC2 now, so whether this goes in or not, no merging for now! Thanks.

@lkishalmi

Copy link
Copy Markdown
Contributor Author

So for me, the steps to reproduce:

$ export JAVA_HOME=/usr/lib/jvm/java-11-openjdk-amd64/
$ git log -1 |head -1
commit ee088f97727c3f365ccd007ac40ebbf3a9c1caa2
$ ant build -Dcluster.config=java
$ ant -f java/gradle.java test-single '-Dtest.includes=**/VerifyGradleProjectTemplatesTest*'
    [junit] Test org.netbeans.modules.gradle.VerifyGradleProjectTemplatesTest FAILED

BUILD FAILED

I'm going to test it with JDK 8 as well...

@lkishalmi

Copy link
Copy Markdown
Contributor Author

The test passed with:

export JAVA_HOME=/usr/lib/jvm/java-8-openjdk-amd64/

@lkishalmi

Copy link
Copy Markdown
Contributor Author

I think the ScriptEngine behavior change between Java 8 and 11 could be the cause of this issue.

@JaroslavTulach

JaroslavTulach commented Nov 4, 2021

Copy link
Copy Markdown

There shall be a (Travis, Actions, Jenkins, etc.) job testing the Gradle modules also on JDK 11.

As far as the VerifyGradleProjectTemplatesTest goes, there is a difference in behavior on JDK-11. The following code is activated on JDK-11 when executing assertProjectAction(prj, ActionProvider.COMMAND_BUILD):

                } catch (IOException ex) {
                    io.getErr().println(Bundle.NO_PLATFORM(ex.getMessage()));
                    gradleTask.finish(1);
                    return;
                }

e.g. there is no platform. That is likely caused by the JavaPlatformImpl being one folder up because System.getProperty("jdk.home") property is set to jdk-11/... Surprisingly the wrong assignment happens in core platform. The following change fixes the problem on JDK-11:

diff --git a/platform/core.startup/src/org/netbeans/core/startup/Main.java b/platform/core.startup/src/org/netbeans/core/startup/Main.java
index 569fa9e182d3..5ef02aa4e052 100644
--- a/platform/core.startup/src/org/netbeans/core/startup/Main.java
+++ b/platform/core.startup/src/org/netbeans/core/startup/Main.java
@@ -204,7 +205,11 @@ public final class Main extends Object {
         jdkHome = System.getProperty("java.home");  // NOI18N
 
         if (!BaseUtilities.isMac()) {
-            jdkHome += File.separator + "..";  // NOI18N
+            File jdkUp = new File(jdkHome).getParent(); // NOI18N
+            File bin = new File(jdkUp, "bin"); // NOI18N
+            if (new File(bin, "javac").exists() || new File(bin, "javac.exe").exists()) { // NOI18N
+                jdkHome = jdkUp.getPath();
+            }
         }
 
         System.setProperty("jdk.home", jdkHome);  // NOI18N

I think the real change is going to need a bit more love. Certainly more desirable than commenting tests out just because they haven't yet been tested on JDK-11 properly.

@JaroslavTulach
JaroslavTulach self-requested a review November 4, 2021 06:38

@JaroslavTulach JaroslavTulach left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Let's run our tests also on JDK-11.

@matthiasblaesing

Copy link
Copy Markdown
Contributor

Flaky tests are a major PITA. I'm at a point where I mostly ignore failing unittests from travis when only the java modules show errors, as currently there is a 100% hit rate on false positives for me. So if noone steps up to fix a test, I think shooting it down is a valid option. Just my 2cents.

@JaroslavTulach

Copy link
Copy Markdown

Flaky tests are a major PITA.

Right, I encourage everyone to add @RandomlyFailing to any test that is randomly failing. If the test succeeds once, fails second time, etc. let us, without discussion, exclude the test from CI runs by adding @RandomlyFailing annotation.

I'm at a point where I mostly ignore failing unittests from travis when only the java modules show errors,
as currently there is a 100% hit rate on false positives for me.

Yes, that's very annoying. Such a flaky tests should be executed only by "Java editor" guys, not everyone. We don't want to fix buggy Java editor code, we only want to know whether we cause regressions there by our own changes. Adding @RandomlyFailing annotation gets us into such state incrementally. The other option is to exclude the whole module from CI testing - I'd rather delay such decision and rather start using @RandomlyFailing aggresisvelly.

So if noone steps up to fix a test, I think shooting it down is a valid option. Just my 2cents.

Right. However this case isn't an example of randomly failing test. The test works reliably on JDK-8 and nobody bothered to execute it on JDK-11 yet. It is failing reliably on JDK-11 because of a bug in the platform. It should be fixed, not shoot down. We should have a CI executing the test on JDK-11 too.

@neilcsmith-net
neilcsmith-net changed the base branch from delivery to master November 8, 2021 14:48
@neilcsmith-net neilcsmith-net modified the milestones: 12.6, 13, NB13 Nov 8, 2021
@neilcsmith-net

Copy link
Copy Markdown
Member

Moving off delivery and 12.6 so discussion can continue. 😄

@lkishalmi

Copy link
Copy Markdown
Contributor Author

@JaroslavTulach you are right, test should be fixed instead of ignored. I've filed this one in a frustration, when I found out that I missed a regression in #3293 due to failing tests. Created NETBEANS-6186 for that. Please do not be grumpy about that. (I've pictured you for a moment as Gandalf in the deep mine scene, shouting "You shall not remove my Tests!".)

@sdedic We've changed something around the loading. I shall check it through. this test is failing and creating a web project results stack overflow.

@lkishalmi lkishalmi closed this Nov 8, 2021
@lkishalmi

Copy link
Copy Markdown
Contributor Author

Closed as this was filed in order to help the 12.6 effort.

@lkishalmi lkishalmi removed this from the NB13 milestone Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do not merge Don't merge this PR, it is not ready or just demonstration purposes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants