Prepare CI for NB 22 and bump minimum JDK requirement to 17 #7019

mbien · 2024-01-30T19:06:03Z

bump JDK from 11 to 17 as new minimum requirement for NB 22
- some tests have to stay on 11 which require the nashorn engine
- also see Many tests fail on JDK 11 #4904 for the remaining JDK 8 locked tests
- the version bump does also allow more tests to run, some needed fixing e.g ConvertToRecordPatternTest and ConvertToNestedRecordPatternTest
- this is according to the agreed Minimum JDK build and run policy
bump action versions to fix deprecation warnings
- actions/cache to v4
- actions/setup-node to v4

note: the graal job is still using an JDK 11 equivalent since #6369, this PR does not change this. However, this should unblock future graal updates, see #7013 (comment)

mbien · 2024-01-30T19:51:47Z

well. some jobs did in fact not fail :)

mbien · 2024-01-31T02:28:38Z

fascinating. Out of almost 3k webcommon/javascript2.editor tests, a single completion test is failing:

netbeans/webcommon/javascript2.editor/test/unit/src/org/netbeans/modules/javascript2/editor/JsCodeCompletionGeneralTest.java

Lines 67 to 69 in 805b153

    
           public void testIssue215777_02() throws Exception { 
        
               checkCompletion("testfiles/completion/issue215777.js", "var x=Math.^", false); 
        
           }

matthiasblaesing · 2024-01-31T18:52:25Z

fascinating. Out of almost 3k webcommon/javascript2.editor tests, a single completion test is failing:

netbeans/webcommon/javascript2.editor/test/unit/src/org/netbeans/modules/javascript2/editor/JsCodeCompletionGeneralTest.java

Lines 67 to 69 in 805b153

public void testIssue215777_02() throws Exception {

checkCompletion("testfiles/completion/issue215777.js", "var x=Math.^", false);

}

I think that we are seeing a scanning effect here. Even for JS this is bogus:

netbeans/webcommon/javascript2.editor/test/unit/data/testfiles/completion/issue215777.js

Lines 1 to 2 in 805b153

    
           var x= Math. 
        
           var x=Math.

I assume that the indexer picks up the Math.var (End of line 1 + beginning of line 2) and reports that. The tested issue deals with the space between = and Math, so I suggest to split the two cases and this should clear up.

Patch (wrapped in ZIP because GH in its infitite wisdom rejects my patch file...)

0001-Stabilize-JS-Test-testIssue215777.zip

I was able to reproduce the problem locally and this fixed it for me.

mbien · 2024-01-31T23:02:32Z

thanks @matthiasblaesing, I added your commit - everything should be green now. The cause for this might be that the method order is slightly different.

mbien · 2024-01-31T23:04:28Z

harness/nbjunit/src/org/netbeans/junit/MethodOrder.java

        if (!"natural".equals(orderS)) { // NOI18N
            try {
+                // TODO: ClassLoader fields can not be accessed via reflection on JDK 12+
+                // see jdk.internal.reflect.Reflection#fieldFilterMap
+                // this won't work
                Field classesF = ClassLoader.class.getDeclaredField("classes"); // NOI18N


luckily nothing seems to be using test case ordering: https://github.com/search?q=repo%3Aapache%2Fnetbeans%20NbTestCase.order&type=code

so I haven't bothered to fix it. (I am also not sure if there is an easy fix, this might require instrumentation, a custom CL or some hooks into junit)

mbien · 2024-01-31T23:08:47Z

java/java.hints.test/src/org/netbeans/modules/java/hints/test/api/HintTest.java

+    /**Sets a source level for all Java files used in this test.
+     *
+     * @param sourceLevel the source level to use while parsing Java files
+     * @return itself
+     */
+    public HintTest sourceLevel(int sourceLevel) {
+        assertTrue(sourceLevel >= 8);
+        return this.sourceLevel(Integer.toString(sourceLevel));
+    }
+
    /**Sets a source level for all Java files used in this test.
     *
     * @param sourceLevel the source level to use while parsing Java files
     * @return itself
     */
    public HintTest sourceLevel(String sourceLevel) {
+        try {
+            int valid = sourceLevel.startsWith("1.") ? Integer.parseInt(sourceLevel.substring(2)) : Integer.parseInt(sourceLevel);
+        } catch (NumberFormatException ex) {
+            throw new IllegalArgumentException(ex);
+        }
        this.sourceLevel = sourceLevel;
        return this;
    }


some tests used SourceVersion.latest().name() as source level which would be the full enum name. This would be quietly ignored and cause weird issues.

The expected format is now enforced and the tests were fixed which used it wrong.

mbien · 2024-01-31T23:14:56Z

...java.hints/test/unit/src/org/netbeans/modules/java/hints/jdk/ConvertToRecordPatternTest.java

-                        + "        if (s instanceof Person(String name, int s1) p) {\n"
+                        + "        if (s instanceof Person(String name, int s1)) {\n"


@lahodaj i think the tests were wrong? This doesn't seem to be a valid syntax when it also includes the field name.

Another side effect of this PR is that more tests are now active.

matthiasblaesing

Looks sane to me. Thank you.

mbien · 2024-02-05T19:38:27Z

going to squash soon and make this ready to merge. Plan is to merge short after the next delivery->master sync.

mbien · 2024-02-07T22:17:46Z

NB 22 will be released when JDK 22 is already out, I am wondering if we should:

a) change matrix to [17. 21, 22ea]
b) change matrix to [17, 22ea] since we can assume JDK 21 will do fine if it works on 22 (add 21 again once we are at 23ea)

I am slightly in favor of a) even though it wastes a bit more resources, since one release from now the delta between 21 and current will grow, then we will have to run the a) variant anyway.

neilcsmith-net · 2024-02-07T22:23:55Z

@mbien I'm also in favour of a) because that is the approach agreed in the minimum JDK support vote. Of course, if there are tests that consume a lot of resources, we might consider something different on a case-by-case basis.

mbien · 2024-02-09T19:23:29Z

rebased on latest master and fixed conflicts from the just synced delivery branch
squashed
added 22-ea to the matrix

merging once green assuming nothing else shows up

mbien · 2024-02-09T20:45:58Z

i think this will cause too many failures atm. The commit validation matrix would produce 5 runs, combined with the java debugger job at 3 runs we would have to play whack-a-mole a bit too much, I already restarted twice and it is failing again.

~~I am going to replace some 21 runs with 22-ea instead of adding 22-ea as extra run. Retry scripts are already in use so the option is spent.~~

I found a hack to exclude matrix elements conditionally by the fake ternary operator of the expression language.

this opens more options, lets try to find a compromise.
The idea is to test more in label-controlled PRs, but only min/max requirements on master so that we don't have to restart everything all the time.

build job: [17, 21, 22-ea] build is stable and fast - no need to change anything
CV job: [linux, mac, win] on [17] + [linux] on [21], so that we have 4 runs again (22-ea doesn't work yet due to org.graalvm.polyglot.* not being ready)
build-tools and debugger: ['17', '21', '22-ea'] in PRs, [17, 22-ea] on master
java hints: [batch1, batch2] on [17, 21] in PRs and just [batch1, batch2] on [17] on master, this is new, it only tested on [17] before. after Upgrade to JDK 22 javac (build 33). #6968 we can bump this to 22-ea as upper limit

added retry script to CV step and even more fixed tests so that hints tests can run on 21

- add 22-ea to the matrix - exclude some matrix elements for master, this frees more for label-controlled PRs without worsening the whack-a-mole factor bump action versions to fix deprecation warnings: - actions/cache to v4 - actions/setup-java to v4 - actions/setup-node to v4 add retry script to - ide/libs.git - ide/notifications - commit-validation run some tests on JDK 11 which don't work on 17+ tests which require a js script engine: - many platform/(api.)templates tests require js - profiler/profiler.oql OQL does also require js, - BindingsNGTest in webcommon/api.knockout and - TemplatesMimeTypeTest in platform/openide.loaders too other reasons - platform/core.network fails on 17+

fix CanProxyToLocalhostTest: Proxy#toString changed slightly fix JavaCompletionTask121FeaturesTest: add Record to test data fix ConvertToRecordPatternTest and ConvertToNestedRecordPatternTest - enforce parsable source level for hint tests - update record patern syntax in tests fix BroadCatchBlockTest: use stable/jdk-independent test files - jdk keeps removing throws declarations from methods, esp when they extend RuntimeException -> lets make our own test Klasses Stabilize JS Test testIssue215777 by Matthias - split golden files into two NbTestCase method ordering is always using "natural" order - ClassLoader fields can not be accessed via reflection on JDK 12+ - test method ordering options are not used anywhere atm - disable tests Co-authored-by: Matthias Bläsing <mblaesing@doppel-helix.eu>

mbien · 2024-02-10T02:51:27Z

all green, merging

mbien added CI continuous integration changes ci:all-tests [ci] enable all tests labels Jan 30, 2024

mbien added this to the NB22 milestone Jan 30, 2024

mbien force-pushed the ci-update-for-nb22 branch 2 times, most recently from d428701 to c1dbdbd Compare January 31, 2024 00:49

mbien force-pushed the ci-update-for-nb22 branch 2 times, most recently from 51fdf0b to 552f4f8 Compare January 31, 2024 22:56

mbien commented Jan 31, 2024

View reviewed changes

mbien marked this pull request as ready for review January 31, 2024 23:27

mbien requested review from matthiasblaesing, neilcsmith-net, dbalek, sdedic and lahodaj January 31, 2024 23:27

mbien added the Need Squashing label Jan 31, 2024

mbien mentioned this pull request Jan 31, 2024

Update the mx suite parsing to work with sources of GraalVM version 23.1.0 #7013

Merged

mbien added tests Upgrade JDK Upgrade to the JDK requirements of a module. labels Feb 1, 2024

mbien mentioned this pull request Feb 2, 2024

Update Gradle Tooling API wrapper to 8.5 #7022

Closed

matthiasblaesing approved these changes Feb 3, 2024

View reviewed changes

mbien force-pushed the ci-update-for-nb22 branch from 552f4f8 to f2079bc Compare February 9, 2024 19:12

mbien removed the Need Squashing label Feb 9, 2024

mbien force-pushed the ci-update-for-nb22 branch 3 times, most recently from 27a8fdf to 596ba28 Compare February 9, 2024 23:19

mbien and others added 2 commits February 10, 2024 02:34

mbien force-pushed the ci-update-for-nb22 branch from 596ba28 to a0109cc Compare February 10, 2024 01:41

mbien merged commit ff360b6 into apache:master Feb 10, 2024
48 checks passed

mbien mentioned this pull request Feb 10, 2024

Upgrade to JDK 22 javac (build 33). #6968

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare CI for NB 22 and bump minimum JDK requirement to 17 #7019

Prepare CI for NB 22 and bump minimum JDK requirement to 17 #7019

mbien commented Jan 30, 2024 •

edited

Loading

mbien commented Jan 30, 2024 •

edited

Loading

mbien commented Jan 31, 2024

matthiasblaesing commented Jan 31, 2024

mbien commented Jan 31, 2024

mbien Jan 31, 2024 •

edited

Loading

mbien Jan 31, 2024

mbien Jan 31, 2024

matthiasblaesing left a comment

mbien commented Feb 5, 2024

mbien commented Feb 7, 2024

neilcsmith-net commented Feb 7, 2024 •

edited

Loading

mbien commented Feb 9, 2024 •

edited

Loading

mbien commented Feb 9, 2024 •

edited

Loading

mbien commented Feb 10, 2024

		+ " if (s instanceof Person(String name, int s1) p) {\n"
		+ " if (s instanceof Person(String name, int s1)) {\n"

Prepare CI for NB 22 and bump minimum JDK requirement to 17 #7019

Prepare CI for NB 22 and bump minimum JDK requirement to 17 #7019

Conversation

mbien commented Jan 30, 2024 • edited Loading

mbien commented Jan 30, 2024 • edited Loading

mbien commented Jan 31, 2024

matthiasblaesing commented Jan 31, 2024

mbien commented Jan 31, 2024

mbien Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

mbien Jan 31, 2024

Choose a reason for hiding this comment

mbien Jan 31, 2024

Choose a reason for hiding this comment

matthiasblaesing left a comment

Choose a reason for hiding this comment

mbien commented Feb 5, 2024

mbien commented Feb 7, 2024

neilcsmith-net commented Feb 7, 2024 • edited Loading

mbien commented Feb 9, 2024 • edited Loading

mbien commented Feb 9, 2024 • edited Loading

mbien commented Feb 10, 2024

mbien commented Jan 30, 2024 •

edited

Loading

mbien commented Jan 30, 2024 •

edited

Loading

mbien Jan 31, 2024 •

edited

Loading

neilcsmith-net commented Feb 7, 2024 •

edited

Loading

mbien commented Feb 9, 2024 •

edited

Loading

mbien commented Feb 9, 2024 •

edited

Loading