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

Add tests which exercise more complex JUnit extensions #35124

Merged
merged 1 commit into from
Aug 8, 2023

Conversation

holly-cummins
Copy link
Contributor

@holly-cummins holly-cummins commented Jul 31, 2023

We have several scenarios at the moment which don't work, and we don't have tests covering them. I've added (disabled) tests for these scenarios, in preparation for enabling them alongside a fix.

I wasn't totally sure what the best home was. I considered putting these tests in the maven project, test-extension, or in a new project dedicated to the test framework. In the end, I put them in the test-extension project, since it would often be extensions that were defining these kinds of JUnit extensions.

What’s in this PR?

I've checked that when enabled, all of the tests fail for the expected reason. For example, the parameter test triggers a deep clone which can't succeed on Java 17+, so it fails with

---- Debugging information ----
message             : No converter available
type                : java.util.concurrent.CopyOnWriteArrayList
converter           : com.thoughtworks.xstream.converters.reflection.SerializableConverter
message[1]          : Unable to make private void java.util.concurrent.CopyOnWriteArrayList.readObject(java.io.ObjectInputStream) throws java.io.IOException,java.lang.ClassNotFoundException accessible: module java.base does not "opens java.util.concurrent" to unnamed module @4f128c33
converter[1]        : com.thoughtworks.xstream.converters.reflection.ReflectionConverter
message[2]          : Unable to make field private static final long java.util.concurrent.CopyOnWriteArrayList.serialVersionUID accessible: module java.base does not "opens java.util.concurrent" to unnamed module @4f128c33
-------------------------------
        at com.thoughtworks.xstream.core.DefaultConverterLookup.lookupConverterForType(DefaultConverterLookup.java:88)
        at com.thoughtworks.xstream.XStream$1.lookupConverterForType(XStream.java:478)
        at com.thoughtworks.xstream.core.TreeMarshaller.convertAnother(TreeMarshaller.java:49)

I tried to add a test for #30317 (with an extension to bring in parent-first dependencies, a test application, and a test), but even on Quarkus 3.0.0.Alpha4, I couldn’t reproduce the original issue.

Wait, isn’t this change pointless since all the tests are disabled?

Well, sort of. But it’s a biiiiig changeset, and it would make #34681 even bigger if all this was included with all that. That would be no fun to review. Also, it’s useful to have a clean changeset in which I was able to confirm each new test failed if the implementation changes weren’t there.

@quarkus-bot

This comment has been minimized.

@holly-cummins holly-cummins force-pushed the tests-for-testing branch 3 times, most recently from cfec5e8 to ae760b6 Compare August 1, 2023 12:07
@quarkus-bot

This comment has been minimized.

@holly-cummins
Copy link
Contributor Author

I think this is ready for review. There are some failures in Windows on tests that I touched, but I think @aloubyansky mentioned that he'd seen them in other builds?

<maven.compiler.release>17</maven.compiler.release>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
<surefire-plugin.version>3.0.0-M7</surefire-plugin.version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could do the same as for the compiler plugin here.

@aloubyansky
Copy link
Member

@holly-cummins it looks ok to me except points in the test poms, could you check please?

@quarkus-bot

This comment has been minimized.

@holly-cummins
Copy link
Contributor Author

#35195 shows the same JarRunnerIT faillure, so I think it's unrelated to my changes.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot
Copy link

quarkus-bot bot commented Aug 8, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants