Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Undocumented error message that is possibly wrong. #868

Closed
PowerStat opened this issue Oct 25, 2023 · 28 comments
Closed

Undocumented error message that is possibly wrong. #868

PowerStat opened this issue Oct 25, 2023 · 28 comments

Comments

@PowerStat
Copy link

PowerStat commented Oct 25, 2023

Describe the bug
I have a small opensource project here on which I use the equalsverifier first:
https://github.com/PowerStat/TemplateEngine
After adding it (not commited yet) I got the following error message:

[ERROR] Failures:
[ERROR] TemplateEngineTests.equalsContract:1966 EqualsVerifier found a problem in class de.powerstat.phplib.templateengine.TemplateEngine.
-> Unable to make field private final java.util.Map de.powerstat.phplib.templateengine.intern.VariableManager.vars accessible: module de.powerstat.phplib.templateengine does not "opens de.powerstat.phplib.templateengine.intern" to unnamed module @44a7bfbc

For more information, go to: https://www.jqno.nl/equalsverifier/errormessages
(EqualsVerifier 3.15.2, JDK 17.0.8 on Windows 10)

The point is the "intern" package should not be visible o the outside. I also can't see a bug here that could explain this - but maybe I am overseeing something?
Also the message is not documented on the mentioned errormessages page.

To Reproduce
Checkout my repo, add "requires nl.jqno.equalsverifier;" to the "module-info.test".
Add
"<dependency>
<groupId>nl.jqno.equalsverifier</groupId>
<artifactId>equalsverifier</artifactId>
<version>3.15.2</version>
<scope>test</scope>
</dependency>" to the pom.xml

Modify the TemplateEngineTests.java:

Add:
@test
public void equalsContract()
{
EqualsVerifier.forClass(TemplateEngine.class).verify();
}

Add:
@disabled("EqualsVerifier")

to testHashCode() and testEquals()

Do "mvn clean test".

Code that triggers the behavior
See above

Error message
See above

Expected behavior
No error message and everything is ok.

Version
See above: 3.15.2

Additional context
Java 21 is also installed on my system.

@PowerStat PowerStat changed the title Undocumented error message that is wrong. Undocumented error message that is possibly wrong. Oct 25, 2023
@jqno
Copy link
Owner

jqno commented Oct 26, 2023

As mentioned in #867, I don't have much experience running EqualsVerifier as a module.

However, EqualsVerifier uses reflection to modify the class that you're testing, and also all of its dependencies. Therefore, they must all be open for reflection, at least for the purporses of EqualsVerifier. Or you can provide instances of this class (VariableManager) via .withPrefabValues().

I agree that I should probably add this to the list of error messages on the website.

@PowerStat
Copy link
Author

For the case the the module path is the problem I have two ideas:

  1. Adding an extra "exports de.powerstat.phplib.templateengine.intern;" to the test/java/module-info.test
    This seems not to work after a short test.
  2. Would it be possible to transform equalsVerifier to be a test code generator instead of using reflections? The question I have in background here is: would equalsVerifier work with GraalVM native images?

@jqno
Copy link
Owner

jqno commented Oct 27, 2023

I'm able to reproduce the problem, but I don't know how to solve it yet.

I've added a simpler class to your project, in the same package as the TemplateEngine:

public class Point {
private final int x;
public Point(int x) { this.x = x; }
}

That results in java.lang.reflect.InaccessibleObjectException: Unable to make field private final int de.powerstat.phplib.templateengine.Point.x accessible: module de.powerstat.phplib.templateengine does not "opens de.powerstat.phplib.templateengine" to unnamed module @d86a6f

So adding prefab values for the fields of the class won't help, the class under test itself must also be open to EqualsVerifier.

For now I can offer you only two workarounds:

  1. Run the tests on the classpath instead of the modulepath, or
  2. Open the opens de.powerstat.phplib.templateengine for reflection.

Before you opened this ticket, I was already looking into two things:
a) finding ways to have less reflection in EqualsVerifier. Unfortunately this isn't an easy problem to solve, and sadly I have little time to devote to it, having a job and a family.
b) if it's possible to run EqualsVerifier on the modulepath, see what the consequences are, and find ways to solve any problems. I guess your project is a good template to do some research on. Can you please share with me the parent pom for your project? I'm very curious to see how you've set this up, because I haven't seen any projects before which run the unit tests on the modulepath. This will also help me improve the documentation as you requested in #867.

@PowerStat
Copy link
Author

For the case the the module path is the problem I have two ideas:

  1. Adding an extra "exports de.powerstat.phplib.templateengine.intern;" to the test/java/module-info.test
    This seems not to work after a short test.
  2. Would it be possible to transform equalsVerifier to be a test code generator instead of using reflections? The question I have in background here is: would equalsVerifier work with GraalVM native images?

@PowerStat
Copy link
Author

For me I will go with workaround option 2, because to my best knowledge option 1 is not an option, because the classpath applies to be deprecated - or did they revert this?
You can find the parent pom here: https://github.com/PowerStat/ToolbaselineJava

Btw. you are not the only one with job and family ;-) Today its the birthday of one of my grandchilds ;-)

@jqno
Copy link
Owner

jqno commented Oct 27, 2023

I wouldn't say that the classpath is deprecated, far from it. Although it might be good to look to the future. I've encountered many projects that use modules, but yours is the first that also runs the tests as modules.

That said, I can't find in your parent pom how you set that up. Can you maybe show me where to look exactly so I can reproduce this in a smaller test project?

Congrats for your grandchild!

@scordio
Copy link
Contributor

scordio commented Oct 27, 2023

JUnit Pioneer also runs tests on the module path and uses EqualsVerifier 🙂

@jqno
Copy link
Owner

jqno commented Oct 27, 2023

@scordio That's awesome! 😃

Can you point me to the relevant portion of the build script that makes the tests run on the modulepath?

@scordio
Copy link
Contributor

scordio commented Oct 27, 2023

@jqno
Copy link
Owner

jqno commented Oct 27, 2023

I have read something he wrote on the subject, but I'm not sure if it was that one. Like like a great reference though, I'll be sure to read it when I have some time.

And now you mentioned it, he even offered to help in one of the other open tickets. And there aren't even that many. I feel slightly ashamed now 🙈😅 But also more motivated to finally get this module stuff straightened out...

@sormuras
Copy link

Still here. Happy to help. 🤓

@PowerStat
Copy link
Author

I don't know what you are searching for in the parent pom, but there is only a small special about module testing. It still works using the maven surefire plugin. The only special thing I know about is that within src/test/java you could place a module-info.test that will replace (done by surefire) the module-info.java file during testing.

@PowerStat
Copy link
Author

PowerStat commented Oct 27, 2023

I don't know if that helps but there is the following article about modules (in German language) from oracle:
https://www.oracle.com/de/corporate/features/understanding-java-9-modules.html

Maybe there is a translation or google translate might help.

The interesting part (at the end) is that you might need to use "setAccessible" in your code - after my module has been defined to be open - or only the internal package has to be defined as opens.

@PowerStat
Copy link
Author

Transation:

Reflection defaults

By default, a module with reflexive runtime access to a package can see the package's public types (and their nested public and protected types). However, code in other modules can access all types in the specified package and all members within those types, including private members via setAccessible, as in previous versions of Java.

For more information about setAccessible and reflection, see Oracle's documentation.

@jqno
Copy link
Owner

jqno commented Oct 27, 2023

Thanks @sormuras ! 😄

If I read your text correctly, and @PowerStat's comments and code, then there only thing I have to do is add a module-info.java (or module-info.test) file to src/test/java and Surefire will just run everything on the modulepath automatically, is that correct?

In that case, I suppose I could spin off a new Maven submodule, add a module-info.java there, and run some module tests there as part of my build, while keeping the existing tests (in other Maven submidules) on the classpath, is that correct?

If so, I could do some experimenting there, learn the module system by doing, and determine how I should update the documentation.

Then I can introduce a proper module-info.java as part of the EqualsVerifier distribution later (and remove the Automatic-Module-Name entry in MANIFEST.MF) and EqualsVerifier wil be properly modularized.

@jqno
Copy link
Owner

jqno commented Oct 27, 2023

Or will putting a module-info.java file in src/main/java also trigger Surefire to run everything on the modulepath? Because in that case, I should probably be a little more careful, since I don't think EqualsVerifier's tests will work that way in their current state...

@jqno
Copy link
Owner

jqno commented Oct 28, 2023

I've had some time to play around with your project, @PowerStat , and I found a few things. Note, first, that I'm running the tests using Maven, not from IntelliJ.

  • Your tests don't live in the same package as the class under test does (de.powerstat.phplib.templateengine vs de.powerstat.phplib.templateengine.test), somehow this complicates things. I've created a simple class Point with only 1 int field, and if I put the EqualsVerifier test for that in the same package, it works; if I put the EqualsVerifier test for that in the ...tests package, it complains about opening up packages. Is there a reason why you have a separate package for your tests?
  • It seems like your module-info.test isn't being picked up by Maven/Surefire. I can just insert obvious syntax errors and it doesn't care. Once I rename it to module-info.java, it starts doing things. @sormuras , do you know why that is? Is module-info.test treated differently from module-info.java?
  • Actually, when I have a module-info.java in the src/test/java folder, I get a [ERROR] org.apache.maven.surefire.booter.SurefireBooterForkException: There was an error in the forked process error. I haven't been able to figure out why, or how to fix that, so far.

Even when moving the EqualsVerifier test to the same package as the class it verifies, and even with the prefab values, I had to add these lines to the module-info.java:

  opens de.powerstat.phplib.templateengine;
  opens de.powerstat.phplib.templateengine.intern;

I would have expected that adding prefab values for some of the fields would work too, instead of opening up the package. Haven't been able to figure out why, yet. To be continued!

TL;DR for @sormuras : do you know if module-info.test is treated differently from module-info.java?

@sormuras
Copy link

TL;DR for @sormuras : do you know if module-info.test is treated differently from module-info.java?

Yes. It's a total different thing for non-official feature. Only some test plugins (for build tools) support it. Which means, official tools like javac and java and also IDEs ignore it by default.

@jqno
Copy link
Owner

jqno commented Oct 28, 2023

Ah, thanks for clarifying. In @PowerStat's project, module-info.test looks like a typical module-info.java, I feel like that's not correct either, then... Should it contain --add-reads and --add-opens flags instead?

Also, if I provide a module-info.java in the tests, does it replace the original one, it do they get merged?

@jqno
Copy link
Owner

jqno commented Oct 28, 2023

OK, should have read @sormuras's post more closely instead of just skimming it, all of my questions were answered: yes, module-info.test indeed has a very different syntax from a module-info.java and therefore the module-info.test file in @PowerStat's project will never work, and yes, module-info.java in src/test/java will replace src/main/java. Also, the module-info.java in src/test/java must start with open module ..., otherwise JUnit can't do the reflection needed to find tests (and, at least in my case, you get the SurefireBooterForkException).

@PowerStat, to fix your project, here's what you should do. Rename your module-info.test to module-info.java, and make sure it looks like this:

open module de.powerstat.phplib.templateengine {  // Note the 'open'

    // requires java.io;
    // requires java.nio;
    // requires java.util;

    requires org.apache.logging.log4j;
    requires org.junit.jupiter.api;
    requires com.github.spotbugs.annotations;
    requires io.cucumber.java;

    requires org.junit.platform.suite;
    requires io.cucumber.junit.platform.engine;
    requires net.bytebuddy;    // A dependency for EqualsVerifier
    // for some reason, it's not needed to add requires nl.jqno.equalsverifier;
}

That should do it.

I'll assign myself some homework too.

  • EqualsVerifier can give the "does not "opens " to unnamed module" error both for fields in the class under test, and for the class under test itself. The latter can only happen when testing is done on the modulepath, and it needs to be handled slightly differently. I'll update EqualsVerifier to improve the error message in these two cases, to make it more clear what should be done. I'll leave this ticket open until I've done that.
  • I'll update the list of error messages, and perhaps add a chapter to the manual. I'll track the progress of that in Please add info about module requires for equalsverifier to the documentation. #867.

@PowerStat, thanks for providing me with a nice rabbit hole to dive into over this weekend, and @scordio and @sormuras, thanks for helping me climb out of it again :).

@PowerStat
Copy link
Author

PowerStat commented Oct 29, 2023

@jqno Sorry for beeing away for some days, but I try to answer open questions here:

  1. I put the tests into a separate "test" package to eliminate the accessibility of package level - thats good practice since years. But there might be different opinions about this.

  2. For me here the module-info.test works since a long time if this would not be the case the unit tests as well as the cucumber tests would not work, because they must be required by the module-info: requires org.junit.jupiter.api; requires io.cucumber.java; Just for info - the module-info.test thing has been invented by eclipse (when I remember correctly) to avoid problems. Because I am building mainly on the commandline with pure maven I am 110% sure that this works.
    But when you are building in another IDE it might not work - for example I know that Netbeans uses it's own buildin maven - that could be replaced with a separate maven installation. When using the netbeans buildin maven you have sometimes things that are not working as expected.
    Maybe this url brings light in here:
    https://stackoverflow.com/questions/58979522/production-code-test-module-info-unpossible

  3. That I have to add

    opens de.powerstat.phplib.templateengine;
    opens de.powerstat.phplib.templateengine.intern;

to my module-info.test is something I have expected. Or opening the complete module.

  1. Thanks for your great work on the equalsVerifier - I see it as an very important quality tool.

@jqno
Copy link
Owner

jqno commented Oct 30, 2023

  1. That's a good point, but personally I haven't seen it in practice a lot. It complicates things a little, so I'll document for the "simple" case of having the tests in the same package. The configuration of what packages to open to where, I'll leave up to the reader 😅
  2. I'm running everything from Maven too. I can add blatant syntax errors in the file, or delete it altogether, and the behaviour doesn't change. Please try it if you don't believe me! @sormuras writes, both in his "Testing in the modular world" blog mentioned here, and in the StackOverflow page that you link to, that src/test/java/module-info.test should contain compiler flags, not a Java syntax module definition (because that should go in src/test/java/module-info.java). The reason why everything still works despite this, I guess (and let me emphasise the word guess) is because the tests accidentally run on the classpath instead of the modulepath, since they live in a different package (..tests) that's unknown when src/main/java/module-info.java is compiled, and the module description in src/test/java is ignored because it's malformed. Your original error message seems to support that: it says module de.powerstat.phplib.templateengine does not "opens de.powerstat.phplib.templateengine.intern" to unnamed module @44a7bfbc -- note that it says unnamed module, not de.powerstat.phplib.templateengine.
  3. OK
  4. Thank you! That's always nice to hear 😄

@PowerStat
Copy link
Author

@jqno
Thanks for making point 2. clear - lucky me we have a long (4 days) weekend here in Germany - so I looked deeply into it - and you are right I was on the wrong path with the module-info.test for 4 years - and there was no hint/error message etc. on it.
So I have to fix this complete module test stuff on all my projects ...
To close the devils circle - when I add a module-info.java to src/test/java then eclipse still starts to till out and during saving this file it cuts out some characters without reason ... but thats another story for another day and someone else ;-)

@jqno
Copy link
Owner

jqno commented Oct 30, 2023

Ouch - I wish you good luck with that. Hope it's not too much work to sort this out!

@PowerStat
Copy link
Author

PowerStat commented Oct 30, 2023

@jqno
There is one more thing I remember about having tests in a separate test package - there was something that split packages are not allowed within the modular world. Maybe this is important or maybe I am on a wrong path here too ...

@jqno
Copy link
Owner

jqno commented Oct 30, 2023

I think the split package issue occurs when two different modules export classes in the same package. But in this case, it's the same module, as src/test/java/module-info.java effectively replaces src/main/java/module-info.java and 'production' classes and test classes end up in the same module. So it shouldn't be a problem in this case -- if I understand everything correctly.

@PowerStat
Copy link
Author

@jqno: I only hat the *.test packages in mind - that was the one of the reasons I put the tests into extra subpackages - to avoid having split packages when not integrating the main src with the test src ;-)
But anyway I made a halloween release of the templateengine (as well as the parent pom), so that you could hopefully use it for testing - and for me I hopefully have some more feedback if this module stuff is correct now ....

@jqno
Copy link
Owner

jqno commented Nov 1, 2023

I've just released version 3.15.3, which improves the readability of the error messages. I'll now close this issue. If you have any other problems or questions, don't hesitate to open a new one! And good luck with the changes!

@jqno jqno closed this as completed Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants