-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Conversation
There was a problem hiding this 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!
There was a problem hiding this 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
Also, I'm pretty sure this will close more outstanding issues |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@dmlloyd I was able to reproduce the problem locally with 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 |
It looks like it's having a problem because the |
Remove usage of xstream or serialization for cloning, and use the JBoss Marshalling cloner instead. Fixes quarkusio#15892.
Status for workflow
|
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): |
Xstream goes poof? That's excellent news. |
I think that cleaning up the class loader situation is still a worthy goal. |
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 |
Relates to quarkusio#40601, quarkusio#40749, quarkusio#38991. No use waiting for Dependabot. Fixes problem where an NPE can occur in some tests.
…#40749" This reverts commit fc3988b. It has some additional changes which re-revert part of quarkusio#40601, to reintroduce removed guards to avoid cloning things like Quarkus runtime classes. Otherwise we get test failures.
…#40749" This reverts commit fc3988b. It has some additional changes which re-revert part of quarkusio#40601, to reintroduce removed guards to avoid cloning things like Quarkus runtime classes. Otherwise we get test failures.
…#40749" This reverts commit fc3988b. It has some additional changes which re-revert part of quarkusio#40601, to reintroduce removed guards to avoid cloning things like Quarkus runtime classes. Otherwise we get test failures.
…#40749" This reverts commit fc3988b. It has some additional changes which re-revert part of quarkusio#40601, to reintroduce removed guards to avoid cloning things like Quarkus runtime classes. Otherwise we get test failures.
…#40749" This reverts commit fc3988b. It has some additional changes which re-revert part of quarkusio#40601, to reintroduce removed guards to avoid cloning things like Quarkus runtime classes. Otherwise we get test failures.
…#40749" This reverts commit fc3988b. It has some additional changes which re-revert part of quarkusio#40601, to reintroduce removed guards to avoid cloning things like Quarkus runtime classes. Otherwise we get test failures.
…#40749" This reverts commit fc3988b. It has some additional changes which re-revert part of quarkusio#40601, to reintroduce removed guards to avoid cloning things like Quarkus runtime classes. Otherwise we get test failures.
Remove usage of xstream or serialization for cloning, and use the JBoss Marshalling cloner instead.
Fixes #15892.