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

[SUREFIRE-1629] Added integration test for surefire-1629 #299

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[SUREFIRE-1629] Added integration test for surefire-1629 #299

wants to merge 5 commits into from

Conversation

cowwoc
Copy link
Contributor

@cowwoc cowwoc commented May 21, 2020

No description provided.

@cowwoc
Copy link
Contributor Author

cowwoc commented May 21, 2020

The interesting bit is if you remove module1, module2 from the testcase (convert it to a single-module project) then Surefire returns the compiler error to the console without crashing. The presence of modules causes it to crash for some reason.

@Tibor17
Copy link
Contributor

Tibor17 commented May 23, 2020

I am waiting for our build process to complete. Then i will cherry pick your last three commits and i will execute it localy. Let's see what will happen afterwards...

@Tibor17
Copy link
Contributor

Tibor17 commented May 24, 2020

@cowwoc
I have the result after running the test on our fix in #293 .
The problem is that your package is named java. You should not create such of folder module2/src/test/java/java/testcase and create e.g. module2/src/test/java/testcase.
This is the error found in the dump:

Error occurred during initialization of boot layer
java.lang.LayerInstantiationException: Class loader (instance of):
'app' tried to define prohibited package name: java.testcase

@cowwoc
Copy link
Contributor Author

cowwoc commented May 25, 2020

@Tibor17 This is intentional. I am expecting to get the same error message in forked-mode as in non-forked mode. Meaning, Surefire should not crash/dump. It should output the compiler error as it does in non-forked mode.

@Tibor17
Copy link
Contributor

Tibor17 commented May 30, 2020

It is a little complicated.
So, the latest commit in master is able to print the message Error occurred during initialization of boot layer.

Normally the plugin is using the process pipes however you wont see these problems with tcp/ip channel used by the forked process.
The only issue why this channel is not enabled by default is the reason that we introduced it in current version and the master branch and we want to prevent from using it by 100% of users. It's better to let it used by little percentage and fix some issues however we do not expect any. We will enable the tcp/ip channel in the last milestone version. This is how you can enable the tcp/ip:

<configuration>
  <forkNode implementation="org.apache.maven.plugin.surefire.extensions.SurefireForkNodeFactory"/>
</configuration>

@cowwoc
Copy link
Contributor Author

cowwoc commented May 30, 2020

@Tibor17 Sounds good. Can you please make the integration test pass with this configuration and leave a note to remove it when the implementation becomes default?

@elharo elharo changed the title Added integration test for surefire-1629 [SUREFIRE-1629] Added integration test for surefire-1629 Feb 10, 2021
@Tibor17
Copy link
Contributor

Tibor17 commented Jan 22, 2022

@cowwoc
We have several integration tests which are using <forkNode implementation="org.apache.maven.plugin.surefire.extensions.SurefireForkNodeFactory"/>. For instance see the tests in ConsoleOutputIT. Truly it is hard to simulate native calls with standard output, for instance JVM and GC of JPMS uses native calls in order to print the JVM warning (regarding JVM warnings Error occurred during initialization of boot layer) on the standard output and error. One way would be java.io.FileDescriptor.out obviosly refers to the native calls, but I do not use to show it in the public audience. If you would try to use native calls in new integration test, then it would confirm the fix with <forkNode implementation="org.apache.maven.plugin.surefire.extensions.SurefireForkNodeFactory"/>.

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

Successfully merging this pull request may close these issues.

2 participants