LUCENE-10328: Module path for compiling and running tests is wrong #571

dweiss · 2021-12-27T09:57:41Z

No description provided.

…ource folders.

…ith modules/

…st*)

… test fixtures plugin provides another layer of complexity to managing dependencies - the gain is so minimal here that I removed it.

…rcular dependncies can be figured out.

…ally todos.

…broken with modules/" This reverts commit 7743921.

…adle itself).

…headFilter so that it's not picked up as a test.

… is a sad case of difference in how javac and ecj consider classes on module path vs. classpath

…lasses, resources, etc.

…ularity.

uschindler · 2022-01-03T10:09:41Z

Hi @dweiss,
I merged main and reverted some changes in this PR that were solved differently than in other branch #567.

uschindler · 2022-01-03T10:19:05Z

There is now a warning by ECJ, but does not fail build. I will fix this in main, sorry.

uschindler · 2022-01-03T10:19:50Z

Sorry, github was too slow and I accidentally pressed button twice.

uschindler · 2022-01-03T10:37:42Z

Fixed warning

uschindler

I do not fully understand all changes, but I hope that's fine. Everyday something new with modules.

At moment I have a few questions:

When are tests run as separate module (integration tests)? Is this decided based on the gradle name or the existence of a module-info.java inside the test's source dir?
When do we use module patching (I still see sometimes patching, but haven't figured out when it is done)? Do we run tests always inside the main sourcesets module (patching) or do we just run those in classpath (when the tests have no module-info.java?)

If you have some time, we can do a Zoom meeting to go through this patch. I am a bit lost.

I trust you so maybe commit it and I will se how it behaves.

gradle/validation/validate-source-patterns.gradle

lucene/core.tests/src/java/module-info.java

gradle/java/modules.gradle

dweiss · 2022-01-05T10:36:40Z

Please take a look at this comment/ chart, Uwe.
https://issues.apache.org/jira/browse/LUCENE-10328?focusedCommentId=17468676&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17468676

How tests are run depends on the combination of whether source and test source sets are modules.

We do not have module patching. I don't think it's possible to configure gradle internal infrastructure and plugins to reasonably use it.

uschindler · 2022-01-05T15:36:39Z

Please take a look at this comment/ chart, Uwe. https://issues.apache.org/jira/browse/LUCENE-10328?focusedCommentId=17468676&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17468676

How tests are run depends on the combination of whether source and test source sets are modules.

We do not have module patching. I don't think it's possible to configure gradle internal infrastructure and plugins to reasonably use it.

OK thanks for the picture. Much more information. I was hoping it is like that. That module patching does not work was known to me, I just stumbled on some support methods for it and I did not understand when they are used.

I am perfectly fine when we run our tests for a class in classpath mode, although the main sourceset is modular. Technically this won't change the test results, as UNIT tests should only check internal assertions. Of course in reality it is more complicated.

What I understood and which is missing in the image: The depenendecies are always modular, so lucene.core is put on module-path, even so we are running tests in classpath mode. This is waht this PR mainly changes, correct? reviously it was not fully working unless you explicitely declared it.

What we should now do (after this is merged):

Review implementation vs api dependencies (both on gradle and on module-info). With my other PR for test-random-chains i found an issue because of this. E.g. the phonetic module uses commons-codec also in its public API. Compilation of my module worked for some reason, but forbiddenapis failed, as it was not able to see the classes (when inspecting the method signatures). Which is understandable. Also ICU needs to refer to ICU in an API (gradle) / transitive (module-system) way. So we should enable the exports checks. When developing the last patch about logging in core, i would make java.logging still non-transitive, because it is unlikely that you would use it in downstream code (although theres a public signature using JUL). Because of that I added a SuppressWarnings("exports") on the class using it.
Fix up the module-decriptor files to figure out that all is sane

Uwe

gradle/java/modules.gradle

uschindler · 2022-01-05T15:43:41Z

I think you have to solve the conflicts caused by the change for running "gradlew beast". I leave that up to you. Maybe it works better after this branch is merged.

uschindler · 2022-01-05T15:44:21Z

gradle/documentation/render-javadoc.gradle

@@ -57,7 +57,7 @@ allprojects {
      outputDir = project.javadoc.destinationDir
    }

-    if (project.path == ':lucene:luke' || project.path.endsWith(".tests")) {
+    if (project.path == ':lucene:luke' || !(project in rootProject.ext.mavenProjects)) {


What is this mavenProjects?

Ah it is inverse. All projects that will land in Maven central. Yeah thats a better check.

by the way theres also the better operator project !in rootProject.ext.mavenProjects

I used is elsewhere already.

uschindler · 2022-01-05T15:50:37Z

gradle/java/modules.gradle

-    // Configure (tasks.test, sourceSets.test)
-    tasks.matching { it.name == "test" }.all { Test task ->
+    // Lazily configure (tasks.test, sourceSets.test)
+    tasks.matching { it.name ==~ /test(_[0-9]+)?/ }.all { Test task ->


I found that change, so you can merge by using "use mine".

Why can't we lazily find all test tasks using tasks.matching { it instance of Test }.all ?

We shoudl also check that instrumentation with Emma works.

I found that change, so you can merge by using "use mine".

Why can't we lazily find all test tasks using tasks.matching { it instance of Test }.all ?

Or use: https://docs.gradle.org/current/javadoc/org/gradle/api/tasks/TaskCollection.html#withType-java.lang.Class-

dweiss · 2022-01-05T17:17:41Z

The depenendecies are always modular, so lucene.core is put on module-path, even so we are running tests in classpath mode. This is waht this PR mainly changes, correct? reviously it was not fully working unless you explicitely declared it.

lucene.core (and any other dependency placed in modular configurations) is correctly inserted on module-path if this reference is from "outside" the project itself. In other words, the tests within lucene.core run with main source set classes on classpath (otherwise you'd have split package errors) but anywhere else where you reference lucene.core, it will be placed on module-path.

That debug flag (-Pbuild.debug.paths=true) shows verbosely how classpath and module path is configured for each task.

…pache#571)

)

dweiss added 26 commits December 22, 2021 11:29

Non-functional changes cleaning up code

e8435cc

Cleanups and refactorings without functional changes.

aaae8a6

Make sure configurations use jar artifacts instead of split class+res…

7d0b70b

…ource folders.

Work in progress, to be reverted and flattened.

bd0e82b

Convert some more modules to use test framework on module path

7519283

LUCENE-10337: workaround for xerces and split packages.

9fe2855

LUCENE-10335: IOUtils.getDecodingReader(Class<?>, String) is broken w…

7743921

…ith modules/

LUCENE-10338: Scan for tests only by convention file name pattern (Te…

95c5754

…st*)

Follow-up to test convention moved.

b8d445f

Use modular deps in the replicator.

0f8678b

Separate spatial-extras test features into a separate subproject. The…

7e0f1dc

… test fixtures plugin provides another layer of complexity to managing dependencies - the gain is so minimal here that I removed it.

Force certain projects to compile/run test in classpath mode until ci…

6d88a74

…rcular dependncies can be figured out.

Separate spatial-extras test features into a separate subproject.

605ac4d

Separate spatial-extras test features into a separate subproject.

c00fb51

Update locks.

5904448

Tweak the output from source validation. Remove NOCOMMITs, they're re…

dd0fba5

…ally todos.

Revert "LUCENE-10335: IOUtils.getDecodingReader(Class<?>, String) is …

42a7472

…broken with modules/" This reverts commit 7743921.

Permit gradle versions with a matching base version (for debugging gr…

a1f5f14

…adle itself).

Apply Uwe's patch for LUCENE-10335

8f0cbb2

Merge remote-tracking branch 'origin/main' into LUCENE-10328

067c9f4

Tidy.

9b16978

LUCENE-10338: make TestPosition an internal class inside TrivialLooka…

0875392

…headFilter so that it's not picked up as a test.

Multiple-mode rendering of module-path and classpath.

d0abc12

Add oddball hacks for ECJ which otherwise fails with errors. Nekohtml…

d7ff7e1

… is a sad case of difference in how javac and ecj consider classes on module path vs. classpath

Correct javadoc.

66977bc

Remaining fixes to get check to pass

f561e4f

dweiss marked this pull request as draft December 27, 2021 10:00

dweiss added 3 commits December 27, 2021 14:00

Run projects with modular tests with proper modular path - combined c…

3f6069e

…lasses, resources, etc.

Correct jar separation between source sets, add a test validating mod…

df58882

…ularity.

Add a runtime modular path dependency check.

cb420e2

Revert some changes that were better fixed in other PR

c76d65e

uschindler closed this Jan 3, 2022

uschindler reopened this Jan 3, 2022

Merge branch 'main' into LUCENE-10328

66509ff

dweiss added 7 commits January 3, 2022 21:12

Edit comment.

d88f5cf

Merge remote-tracking branch 'origin/main' into LUCENE-10328

1aac2dc

Use test source set constant.

c6363fc

Add decision tree on how to configure classpath/ modular mode.

c175e21

Make morfologik.tests extend from (modular) lucene test-framework.

938e01b

Make core.tests utilize the test framework in a normal, modular way.

d689960

Merge remote-tracking branch 'origin/main' into LUCENE-10328

67ecf7f

dweiss marked this pull request as ready for review January 4, 2022 15:18

dweiss requested a review from uschindler January 4, 2022 15:19

dweiss mentioned this pull request Jan 4, 2022

LUCENE-10352: Convert TestAllAnalyzersHaveFactories and TestRandomChains to a global integration test and discover classes to check from module system #582

Merged

uschindler approved these changes Jan 5, 2022

View reviewed changes

gradle/validation/validate-source-patterns.gradle Show resolved Hide resolved

lucene/core.tests/src/java/module-info.java Show resolved Hide resolved

gradle/java/modules.gradle Show resolved Hide resolved

uschindler approved these changes Jan 5, 2022

View reviewed changes

gradle/java/modules.gradle Show resolved Hide resolved

uschindler reviewed Jan 5, 2022

View reviewed changes

Merge branch 'main' into LUCENE-10328

d1f3732

dweiss merged commit ff547e7 into apache:main Jan 5, 2022

dweiss added a commit to dweiss/lucene that referenced this pull request Jan 5, 2022

LUCENE-10328: Module path for compiling and running tests is wrong (a…

4ecee6c

…pache#571)

asfgit pushed a commit that referenced this pull request Jan 6, 2022

LUCENE-10328: Module path for compiling and running tests is wrong (#571

bc6aa00

)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LUCENE-10328: Module path for compiling and running tests is wrong #571

LUCENE-10328: Module path for compiling and running tests is wrong #571

dweiss commented Dec 27, 2021

uschindler commented Jan 3, 2022 •

edited

uschindler commented Jan 3, 2022 •

edited

uschindler commented Jan 3, 2022

uschindler commented Jan 3, 2022

uschindler left a comment

dweiss commented Jan 5, 2022

uschindler commented Jan 5, 2022

uschindler commented Jan 5, 2022

uschindler Jan 5, 2022

uschindler Jan 5, 2022

uschindler Jan 5, 2022

uschindler Jan 5, 2022

uschindler Jan 5, 2022

uschindler Jan 5, 2022

dweiss commented Jan 5, 2022

LUCENE-10328: Module path for compiling and running tests is wrong #571

LUCENE-10328: Module path for compiling and running tests is wrong #571

Conversation

dweiss commented Dec 27, 2021

uschindler commented Jan 3, 2022 • edited

uschindler commented Jan 3, 2022 • edited

uschindler commented Jan 3, 2022

uschindler commented Jan 3, 2022

uschindler left a comment

Choose a reason for hiding this comment

dweiss commented Jan 5, 2022

uschindler commented Jan 5, 2022

uschindler commented Jan 5, 2022

uschindler Jan 5, 2022

Choose a reason for hiding this comment

uschindler Jan 5, 2022

Choose a reason for hiding this comment

uschindler Jan 5, 2022

Choose a reason for hiding this comment

uschindler Jan 5, 2022

Choose a reason for hiding this comment

uschindler Jan 5, 2022

Choose a reason for hiding this comment

uschindler Jan 5, 2022

Choose a reason for hiding this comment

dweiss commented Jan 5, 2022

uschindler commented Jan 3, 2022 •

edited

uschindler commented Jan 3, 2022 •

edited