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

Test framework: Use JBoss Marshalling cloner #40601

Merged
merged 1 commit into from
May 16, 2024
Merged

Conversation

dmlloyd
Copy link
Member

@dmlloyd dmlloyd commented May 13, 2024

Remove usage of xstream or serialization for cloning, and use the JBoss Marshalling cloner instead.

Fixes #15892.

@dmlloyd dmlloyd requested a review from geoand May 13, 2024 16:50
@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/testing labels May 13, 2024
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Oh, that's some nice cleanup!

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Love it!

I wish I had thought of JBoss Marshaling when we added this

@geoand
Copy link
Contributor

geoand commented May 13, 2024

Also, I'm pretty sure this will close more outstanding issues

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@gsmet
Copy link
Member

gsmet commented May 14, 2024

@dmlloyd I was able to reproduce the problem locally with mvn clean install -Dstart-containers -Dtest-containers in the IT submodule.

In:

@QuarkusTest
public class TransactionalUniAsserterTest {

    private static final AtomicBoolean ASSERTER_USED = new AtomicBoolean();

    @RunOnVertxContext
    @Test
    public void testTransactionalUniAsserter(TransactionalUniAsserter asserter) {
        assertNotNull(asserter);
        asserter.assertThat(Panache::currentTransaction, transaction -> {
            ASSERTER_USED.set(true);
            assertNotNull(transaction);
            assertFalse(transaction.isMarkedForRollback());
            asserter.putData("tx", transaction);
        });
        asserter.assertThat(Panache::currentTransaction, transaction -> {
            assertNotNull(transaction);
            assertFalse(transaction.isMarkedForRollback());
            assertNotEquals(transaction, asserter.getData("tx"));
        });
    }

    @AfterAll
    static void afterAll() {
        assertTrue(ASSERTER_USED.get());
    }

}

we end up with ASSERTER_USED being false.

@dmlloyd
Copy link
Member Author

dmlloyd commented May 14, 2024

It looks like it's having a problem because the TransactionalUniAsserter is getting cloned, and the clone, well, just doesn't work right. I'm digging into the how/why now but it might be a tricky one to solve.

Remove usage of xstream or serialization for cloning, and use the JBoss Marshalling cloner instead.

Fixes quarkusio#15892.
@quarkus-bot
Copy link

quarkus-bot bot commented May 16, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 0e113de.

✅ 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.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ Native Tests - HTTP

📦 integration-tests/rest-client

io.quarkus.it.rest.client.selfsigned.ExternalSelfSignedITCase.should_accept_self_signed_certs_java_url - History

  • Read timed out - java.net.SocketTimeoutException
java.net.SocketTimeoutException: Read timed out
	at java.base/sun.nio.ch.NioSocketImpl.timedRead(NioSocketImpl.java:288)
	at java.base/sun.nio.ch.NioSocketImpl.implRead(NioSocketImpl.java:314)
	at java.base/sun.nio.ch.NioSocketImpl.read(NioSocketImpl.java:355)
	at java.base/sun.nio.ch.NioSocketImpl$1.read(NioSocketImpl.java:808)
	at java.base/java.net.Socket$SocketInputStream.read(Socket.java:966)
	at org.apache.http.impl.io.AbstractSessionInputBuffer.fillBuffer(AbstractSessionInputBuffer.java:161)
	at org.apache.http.impl.io.SocketInputBuffer.fillBuffer(SocketInputBuffer.java:82)

@geoand geoand merged commit 8f946ab into quarkusio:main May 16, 2024
51 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.12 - main milestone May 16, 2024
@dmlloyd dmlloyd deleted the jbmar branch May 16, 2024 03:31
@holly-cummins
Copy link
Contributor

holly-cummins commented May 16, 2024

Ohhhh, nice. The xstream serialization was becoming increasingly non-viable, since it doesn't work on Java 17+, and that's our minimum Java level. This possibly means I don't need to pursue my epic test classloading rewrite (#34681). I was going to pick that work up tomorrow, too (although in fairness, it's been Real Soon Now for like a year).

Here's my (partial) list of issues that I think this might resolve:

Thinking about it more, I think "load the tests with the classloader we intend to use to run them" is probably still a nice aspiration, and trying to avoid serialization could have some broader benefits. I think the following issues probably are not fixed by this change, and would need the epic test classloading rewrite (ETCR):

@maxandersen
Copy link
Contributor

Xstream goes poof? That's excellent news.

@dmlloyd
Copy link
Member Author

dmlloyd commented May 16, 2024

I think that cleaning up the class loader situation is still a worthy goal.

@holly-cummins
Copy link
Contributor

I'm planning to add a test which reproduces the failure @edeandrea and I saw, so we can assess how common it is. If it's a common one, we should maybe consider temporarily reverting this while we wait for the JUnit upgrade. But in the meantime, some tests which were failing because of the broken xstream serialization are now passing (yay!). So I've raised a PR to un-disable them: #40749

dmlloyd added a commit to dmlloyd/quarkus that referenced this pull request May 21, 2024
Relates to quarkusio#40601, quarkusio#40749, quarkusio#38991. No use waiting for Dependabot. Fixes problem where an NPE can occur in some tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A @QuarkusTest with a ParameterResolver that return a java record, throws an exception
5 participants