LUCENE-10301: make test framework a module (remove split packages)

[dweiss]

See https://issues.apache.org/jira/browse/LUCENE-10301

[rmuir]

@dweiss i tried it out locally, but hit a snag. I'm not even sure it relates to this change?

> Task :lucene:distribution.tests:ecjLintTest FAILED
expansion argument file @/home/rmuir/workspace/lucene/lucene/distribution.tests/build/tmp/ecjLintTest/ecj-inputs.txt does not exist or cannot be read

I've uploaded the full log of what i did in case it helps:
checkfail.txt

[rmuir]

so I ran gradlew check -x test, which succeeded.

And i noticed it created the files in question:

./lucene/distribution.tests/build/tmp/ecjLintTest/ecj-inputs.txt
./lucene/distribution.tests/build/tmp/ecjLintMain/ecj-inputs.txt

So I'm guessing this might be somehow a dependency/race condition issue? Maybe best solved for another issue as I think I can work around it to test out this change.

[uschindler]

If you like I can also quickly hack the StackWalker code, but if you want to merge first: let's do it.
My plan: The TestSecrets setters are only allowed to be called from the exact class that is allowed to set, and the TestSecret getters are allowed to be called from any class below packages in test-framework. This is by the way the same like JDK does it.

[dweiss]

Hi Uwe. I wanted to do it too but eventually.... didn't. I mean - it's clearly stated these internal packages are for internal use only (much like many other methods in Lucene). And they're already not accessible in module-mode. Sure, we can add runtime restrictions for these methods but I see little point in doing so - they are already red-taped all over... Why complicate things?

In the longer perspective, we can simply check inside the internal secrets if we're loaded in module-mode (this ensures isolation) and fail if they're loaded on classpath. This will force people to put the test framework (and lucene) on module path but I don't see a problem with that.

[dweiss]

bq. This is by the way the same like JDK does it.

The JDK doesn't have any runtime checks for this, I believe - it's shifted to the module system to verify accessibility:
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/access/SharedSecrets.java

[uschindler]

bq. This is by the way the same like JDK does it.

The JDK doesn't have any runtime checks for this, I believe - it's shifted to the module system to verify accessibility: https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/access/SharedSecrets.java

Sure I was imprecise. In Java 8 they have this check. The difference: JDK 11 is always encapsulated, but lucene can be used with classpath and people will do this forever, trust me.

So I will add a PR to add this, it's so simple and brings security. Trust me people will always use those methods and here they clearly should never ever do this.

[dweiss]

Trust me people will always use those methods and here they clearly should never ever do this.

Oh, I do believe they will - no doubt about that. This said, I am a rebel who likes to touch the internals from time to time - if you let people know they're doing it without any guarantees, I think it's fine. I'll merge the conflicts against main, maybe later today, and then try to commit it in - will leave you a placeholder for the check there (or feel free to provide a PR to this branch, no problem).

[uschindler]

(or feel free to provide a PR to this branch, no problem)

I can also commit it into the branch if you like. But a PR is also fine, working on it already (its simple, a stub method wont help (as code must be on the method itsself).

[uschindler]

BTW, you're right about the setters. The are one-time only, so no need for checks.

[uschindler]

I think the conflicts are caused by me, it's simple, I can merge in.

[uschindler]

Done.

[rmuir]

Oh, I do believe they will - no doubt about that. This said, I am a rebel who likes to touch the internals from time to time - if you let people know they're doing it without any guarantees, I think it's fine. I'll merge the conflicts against main, maybe later today, and then try to commit it in - will leave you a placeholder for the check there (or feel free to provide a PR to this branch, no problem).

My concern is not about rebels, instead I'd just rather not cause hassle: I'm suspicious of the state of tooling around modules right now :) I can imagine cases (at least for the near future until stuff matures?) where someone is running unit tests or using IDE in classpath mode, but then tries to deploy in module mode for production and gets bit? Obviously, this kind of thing is what integration tests are for, but it still feels a bit trappy.

[uschindler]

Oh, I do believe they will - no doubt about that. This said, I am a rebel who likes to touch the internals from time to time - if you let people know they're doing it without any guarantees, I think it's fine. I'll merge the conflicts against main, maybe later today, and then try to commit it in - will leave you a placeholder for the check there (or feel free to provide a PR to this branch, no problem).

My concern is not about rebels, instead I'd just rather not cause hassle: I'm suspicious of the state of tooling around modules right now :) I can imagine cases (at least for the near future until stuff matures?) where someone is running unit tests or using IDE in classpath mode, but then tries to deploy in module mode for production and gets bit? Obviously, this kind of thing is what integration tests are for, but it still feels a bit trappy.

I have a solution that works quite well. Not with getCallerClass() as this needs a special permission. As we only need class name, I use a plain StackWalker. It just crosschecks that the methods are only called from within test framework. with or without module does not matter:

  private static void ensureCaller() {
    final Optional<String> caller = walker.walk(s -> s.map(StackFrame::getClassName).skip(2).findFirst());
    if (!caller.orElseThrow().startsWith("org.apache.lucene.tests.")) {
      throw new UnsupportedOperationException(
          "Lucene TestSecrets can only be used by the test-framework.");
    }
  }

[uschindler]

This is also not performance critica, as only test framework should ever call any method to get secrets.

[uschindler]

Here is a PR, I will just add one test:
dweiss#14

[uschindler]

Hi, I also fixed a bug in the static initializer of TestSecrets: It used wrong class for initialization. You were able to see this with my new test and running ONLY this test in ISOLATION. See: https://github.com/dweiss/lucene/pull/14/files#diff-201a6e8b6ef8434184e32bf0ac679be1a372f7d95c50e6c76db099644ce47b7dR48

[uschindler]

OK, if you merge that PR in, I am happy! Thanks!

[uschindler]

Thanks!

[uschindler]

Wasn't this merged?

[dweiss]

I've merged it manually from command-line. Closing the PR.