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

Try alternative approach that does not need the @Vetoed annotation or custom event #30

Closed
wants to merge 4 commits into from

Conversation

starksm64
Copy link

See what you think of this approach.

jamezp and others added 3 commits August 9, 2024 16:12
}

void setMethodParameters(MethodParameters methodParameters) {
getManager().bind(ApplicationScoped.class, MethodParameters.class, methodParameters);
Copy link
Author

Choose a reason for hiding this comment

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

Maybe the binding scope should be TestScoped

Copy link
Owner

Choose a reason for hiding this comment

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

That would be my guess as well. I'll pull this down and do some more testing though.

@jamezp
Copy link
Owner

jamezp commented Aug 12, 2024

This doesn't seem to work with the TestScoped. The issue is we need to be able to inject into @BeforeEach and @AfterEach methods. However, I'm not seeing a JUnit callback which exposes those methods. I'll take a better look though as I might just be missing it.

What we might need to end up doing is implementing the BeforeTestExecutionCallback and invoke the TestRunnerAdaptoer.test(). I need to determine the order of events though as it might be too late to discover the parameters at that point.

Regardless of this approach or the original approach, injection does not work for @BeforeEach and @AfterEach.

@starksm64
Copy link
Author

Why do we need to inject into @BeforeEach and @AfterEach methods?

@jamezp
Copy link
Owner

jamezp commented Aug 12, 2024

@starksm64 I can see cases where say you want to invoke like a reset on the server so you'd want to inject a URL. Or in the case of WildFly you can inject something like a ServerManager where you could execute management operations.

That said, we could just not support it for now and consider support for it later. You could really just use instance fields instead.

The TestScoped scope doesn't work because the TestRunnerAdaptor.before() doesn't have the TestScoped enabled, which I suppose makes sense. The ApplicationScoped works, but feels rather broad.

I'm going to do some more thinking on this could work. I do agree requiring an annotation for some parameters is less than ideal. There could be something in JUnit and/or Arquillian that could help avoid that. The issue is really when JUnit wants to resolve parameters of in-container tests.

@starksm64
Copy link
Author

starksm64 commented Aug 12, 2024

We could always just add a new BeforeTest scope and BeforeContext handler. I have not looked too much at the Context/EventContext handling. I'm not the biggest fan of the event dispatch as an spi mechanism that arquillian uses. To me it would be more strait forward to just activate the BeforeContext in JunitEventTestRunnerAdaptor#before and deativate it in after without hooking into observing the @Before event.

@jamezp
Copy link
Owner

jamezp commented Aug 12, 2024

The issue seems to be that adapter.before() doesn't invoke any observers for the Before event for in-container tests on the client side if that makes sense. IoW, JUnit invokes in-container tests on the client side and checks if a parameter can be supplied when invoked. It doesn't require the parameter to be created as that part does happen in the container.

We could always just add a new BeforeTest scope and BeforeContext handler. I have not looked too much as the Context/EventContext handling. I'm not the biggest fan of the event dispatch as an spi mechanism that arquillian uses. To me it would be more strait forward to just activate the BeforeContext in JunitEventTestRunnerAdaptor#before and deativate it in after without hooking into observing the @Before event.

Ha, I was actually about to say something really similar. I was typing it out when I saw this come through :) That maybe we have a beforeTest and afterTest method. I can look at different scoping for sure too. And I fully agree on the how the event dispatch works. It's really hard to follow sometimes for me at least.

@jamezp
Copy link
Owner

jamezp commented Aug 12, 2024

Okay, I've updated to arquillian#608 to work without the @Vetoed annotation. If we like that approach we can go with it and I can file another issue for allowing support in the @BeforeEach and @AfterEach methods. I've got a couple ideas on how that might work.

@jamezp jamezp deleted the branch jamezp:issue312 August 13, 2024 00:56
@jamezp jamezp closed this Aug 13, 2024
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