[ARQ-2231] The JUnit 5 container does not work with manual mode tests

[petrberan]

Resolves #543

Opening this as a draft to discuss the solution as it seems a little hack-y to me @starksm64

https://issues.redhat.com/browse/ARQ-2231
and
#543

The problem there is that Arquillian goes through several phases, each creating and removing contexts (as to why I have no idea, haven't found any docs). The error comes at the moment the BeforeEach method tries to deploy the deployment but is unable to find out just because the proper context has already been deactivated. I can't even activate the context without finding the correct deployment, which I cannot do since the context is deactivated.

In the end, the only solution I was able to find was to not deactivate the context just before the deployment and leave the deactivation for After class. I'm not sure if that would be a problem at all, but all other solutions failed - even closing the context after obtaining the deployment to deploy, seems like by that time the context is needed again and is not activated again.

For that reason I had to introduce the arquillian-junit5-core dependency, that is the closest it can get to the actual deployment without breaking anything

[jamezp]

I don't think we can really have a dependency on arquillian-junit5-core here. That could potentially break JUnit 4 and/or TestNG support. However, I feel this gives a place to look.

There is a chance, though we'd need to test, that we do need the context activated for an undeploy in the @AfterEach or @AfterAll.

One thing I wonder is if we simply just need to activate contexts in JUnit when before/after are invoke. I'm not sure how we do that though.

[petrberan]

Thank you @jamezp . That is true, said dependency made JUnit 4 tests unusable. For that, I replace it with the closest context available, which is Before, but that fails some tests. In the current commit it's commented out

New solution is a single case workaround. There's a new check in the deployment method, which checks if the classContext is active or not. If not, that means only one thing - the deployment is managed and deploy is invoked from BeforeEach method. In such a case, it looks up in the stacktrace to find out which class called the deploy and activates the context to find the deployment.

Deactivating might be needed after the deployment is found, I'll look into that tomorrow. As always, every input is very much welcomed

[petrberan]

PR is now ready to merge, the changes has been tested with WFLY -DallTests and Resteasy and none of the builds broke, so I'd assume it's safe to merge

[rhusar]

As mentioned on the chat, this is a pretty big hack masking the real cause which appears to be that the proper contexts are not activated at the given @ BeforeEach method. Unfortunately I wasn't able to see where this should be fixed properly after giving it a little bit of time. Petr mentioned he is looking at different solution, so let's hold up with it.

[petrberan]

The solution has been reworked, WFLY passes

[jamezp]

This makes sense to me and seems to be working. Thank you @petrberan!

I'm not sure if we need to do the same thing for the interceptBeforeAllMethod and interceptAfterAllMethod methods, but it feels like we should.

[rhusar]

While this fixes the issue, this is problematic as the before and after on the SPI are called twice when run as client.

[rhusar]

Anyway, here's the changes I had locally when I was toying with this and that do fix the problems: #546

[rhusar]

This makes sense to me and seems to be working. Thank you @petrberan!

Perhaps have a look at #546

I'm not sure if we need to do the same thing for the interceptBeforeAllMethod and interceptAfterAllMethod methods, but it feels like we should.

I have the same feeling but that couldn't find the reason why it would be necessary.

[petrberan]

I'll check tomorrow if the interceptBeforeAll and AfterAll are needed

[jamezp]

That's a good point about invoking before() and after() twice. It likely does make sense to go with something more like #546. When I run the reproducer with those commits, I do see before() only invoked once.

[starksm64]

Closing this in favor of #546