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

Quarkus support in testbench #1655

Open
ErrorProne opened this issue Jul 18, 2023 · 17 comments
Open

Quarkus support in testbench #1655

ErrorProne opened this issue Jul 18, 2023 · 17 comments
Labels

Comments

@ErrorProne
Copy link

What do I want

Working quarkus support for pro features like UiUnitTest

Explain your problem

Two things, based on who reads this

Testbench-Devs:
There are multiple problems we found with quarkus support so far:

  1. The route-lookup does not work
  2. Quarkus @TestSecurity (https://quarkus.io/guides/security-testing) feature is not working (similiar to springs @WithMockUser)
  3. I'm not 100% sure about that, but it looks like the BeanInstantiator also has to be wired by hand.

All of those should be solved with the workarounds in [1] but an official out-of-the-box support should be the goal.

The vaadin project and team does such an amazing work and the framework is wonderful to use. The same should hold true for testing.

We will be happy to provide feedback or input to any implementations for this.

Project owner/Pro feature Support:
A little rant: Before switching to Vaadin we talked to a few folks and very explicitly stated that we do test a lot and use quarkus. We do deploy applications into factory lines. These apps have to work stable, no exceptions. Naturally we were very interested in the pro Testbench features and have been told that quarkus is supported. Our experience right now is: that is not the case at all.
We got it working eventually but only by diving into the vaadin code and with help from Marco Collovati in this issue in the quarkus extension here [1].

So far it looks like we solved all the issues in [1] but for a pro feature with official quarkus support and no mention about these problems at all (e.g. https://vaadin.com/docs/latest/integrations/quarkus) is a very frustrating experience. Additionally at least I see testing in 2023 not as an nice-to-have addition.

[1] vaadin/quarkus#121

@tarekoraby
Copy link

Thank you @ErrorProne for the ticket. The TestBench team, I'm sure, will address the technical feasibility of the proposed solution.

But as the product manager, my sincere apologies for any false promises made regarding TestBench support with Quarkus. It is indeed reasonable to expect TestBench to work with Quarkus without the need to resort to any workarounds. And if, for some reason, this support turns out to be impossible or prohibitively expensive to implement, we should clearly emphasize that in the Quarkus and TestBench documentation pages.

In all cases, we will aim to investigate possible ways of improving TestBench + Quarkus support.

@benjaminrau
Copy link

benjaminrau commented Jul 24, 2023

Thanks for your reply @tarekoraby - it would be really awesome to see some of our workarounds to make their way into the official Vaadin Testbench implementation.

I forked the afaik official starter project for Quarkus + Vaadin and extended it with workarounds for all of the issues we identified and fixed so far.

You find it here: https://github.com/AliceAndTheBuilders/pro-starter-flow-quarkus

If some comments reads as offense - lets make me clear that this is not my intention and just read them with positive vibes.

We adressed the following issues so far:

  1. auto discovering and registering vaadin routes
  2. UI Unit Tests for quarkus with authentication support
  3. Using UIUnitTest while having another base test class one may want to extend (which is not UI related)
  4. register and initialize ViewAccessChecker
  5. Login form for Quarkus
  6. @permitAll in Vaadin is a synonym for @RolesAllowed("**") which does not match Quarkus definition

@tarekoraby
Copy link

Thanks for sharing the workarounds 🙏 That's very helpful.

It's the summer holiday season here in Finland, but we will get back to you as soon as we can once the bulk of our developers return and had the opportunity to examine the issue.

@ErrorProne
Copy link
Author

ErrorProne commented Jul 27, 2023

TestBench End-To-End is also not working. Some of the problems seem to be related to how quarkus handles isolation of extensions with different classloaders. It looks like you guys should talk to your quarkus folks about that.

I think that we figured out a workaround. We will update the starter later with the new workaround and keep you posted.

@fjasis
Copy link

fjasis commented Jul 27, 2023

In the starter @benjaminrau mentioned, why is needed for the injected properties to be use in @PostConstruct? it is possible to use them outside of the postconstruct?

@ErrorProne
Copy link
Author

ErrorProne commented Jul 27, 2023

In the starter @benjaminrau mentioned, why is needed for the injected properties to be use in @PostConstruct? it is possible to use them outside of the postconstruct?

I'm not sure if I understand the question. But I guess you are referring to this?
https://github.com/AliceAndTheBuilders/pro-starter-flow-quarkus/blob/v24/src/main/java/com/example/starter/pro/ProtectedView.java#L20

He is using the @Inject on a field level. This means the field is not injected when the constructor runs. That is why he switched to @PostConstruct. You could use constructor injection and than use a normal constructor and access the injects.

@fjasis
Copy link

fjasis commented Jul 28, 2023

Yes, that was my question, sorry i worded it poorly, so this should work rigth?
SecurityIdentityAssociation identity
public testConstructor(SecurityIdentityAssociation identity){ this.identity = identity; }

I'm having trouble formatting it properly, but i think it gets the point across

@ErrorProne
Copy link
Author

ErrorProne commented Jul 28, 2023

Yes, as per https://quarkus.io/guides/cdi-reference#simplified-constructor-injection this should work (given that all other things are right). For the starter example this would be

    transient SecurityIdentityAssociation identity;

    public ProtectedView(SecurityIdentityAssociation identity) {
        this.identity = identity;
        textField.setValue(identity.getIdentity().getPrincipal().getName());
    }

@fjasis
Copy link

fjasis commented Jul 28, 2023

Thanks! i tested it yesterday and it works correctly with JPA-security, what is the reason the field injection needs to be used at post construct with vaadin?
also, as how are you handling logouts?

@ErrorProne
Copy link
Author

Thanks! i tested it yesterday and it works correctly with JPA-security, what is the reason the field injection needs to be used at post construct with vaadin? also, as how are you handling logouts?

This is not really a vaadin specific thing. Field injection works in a way where the container (I guess Quarkus ArC in this case) first instantiates your class through the no args constructor and than sets the injects fields via setters. This basically means that the fields will be injected after the constructor has finished. So when field injection is used you have to opt-in for the @PostContruct event. When constructor injection is used instead you have instant access in the injected values inside the constructor, but you have to set the fields by yourself.

Both are valid methods for injection and have a long history of discussions of pros/cons. As far as I'm aware constructor injection is more favoured by tools like sonarqube etc. since they make the classes for example "easier" testable. But you will find people who vote for each variant.

Do you have anything specific in mind regarding logouts? Also feel free to create an issue on our starter fork (https://github.com/AliceAndTheBuilders/pro-starter-flow-quarkus) so we can keep this issue on topic =)

@fjasis
Copy link

fjasis commented Jul 28, 2023

Issue Created!

@mshabarov mshabarov added the bug label Jul 31, 2023
@benjaminrau
Copy link

benjaminrau commented Jul 31, 2023

@fjasis I created a pull request which adds a logout example: AliceAndTheBuilders/pro-starter-flow-quarkus#2

And from what i see, is Vaadin on Quarkus not managing any sessions itself, which would require cleanup.
In Quarkus when using form auth all session information is on the encrypted cookie as Finn mentioned, thus as soon as it is deleted, all session information is gone.

Assuming that, the example should be sufficient.

@ErrorProne
Copy link
Author

ErrorProne commented Aug 3, 2023

For End-To-End tests a workaround is to change the lookup method for required modules in the tests application.properties (important, it requires .properties even if yaml is used!). For example:

quarkus.class-loading.parent-first-artifacts=com.vaadin:vaadin-testbench-core-junit5,com.vaadin:vaadin-testbench-shared,org.seleniumhq.selenium:selenium-api

For vaadin devs workin on this, please see https://quarkus.io/guides/class-loading-reference#isolated-classloaders

I'lltry to push an example to our forked starter later this week

@benjaminrau
Copy link

benjaminrau commented Aug 3, 2023

My observations on the issue which is worked around as @ErrorProne mentioned are the following per example:

  1. @BrowserTest invokes the JUnit MultipleBrowsersExtension which creates an instance of BrowerTestInfo to resolve the parameter on BrowserTestBase::setBrowserTestInfo
  2. This is an instance of BrowserTestBase which is registered on the isolated class loader
  3. @QuarkusTest invokes the QuarkusTestExtension which somewhat intercepts any paramter resolvers for fx @BeforeEach
  4. On the method io.quarkus.test.junit.QuarkusTestExtension#runExtensionMethod() there are some checks if an parameter needs to be cloned for a reason that is described like: the arguments were not loaded from TCCL so we need to deep clone them into the TCCL because the test method runs from a class loaded from the TCCL
  5. Indeed for the BrowserTestInfo instance the following check results in cloneRequired is true: cloneRequired = runningQuarkusApplication.getClassLoader().loadClass(theclass.getName()) != theclass;
  6. Later the BrowserTestInfo instance is tried to be cloned with xStream which fails since xStream is performing a lot of illegal access on java internals

By instructing Quarkus to NOT use the isolated class loader the check for cloneRequired (5) is false and QuarkusTestExtension will not try to clone the instance.

The following comment on the docs at https://quarkus.io/guides/writing-extensions#using-threads-context-class-loader seems related:

The build step will be run with a TCCL that can load user classes from the deployment in a transformer-safe way. This class loader only lasts for the life of the augmentation, and is discarded afterwards. The classes will be loaded again in a different class loader at runtime. This means that loading a class during augmentation does not stop it from being transformed when running in the development/test mode.

Sidenote related to the autodiscover routes feature on Testbench Unit Tests:
Since beeing aware of the class loader topic i think that this class loader issue is also accountable for the issue that no routes are registered on unit tests as we pointed out earlier on this issue.

@mshabarov
Copy link
Contributor

Thanks @benjaminrau , @ErrorProne for providing your codes and all who commented on this issue.
I just want to share our plans about further work - Vaadin Flow team keeps an eye on this topic and we are going to start working on it step-by-step having UI Unit Testing better integration first and then checking the end-to-end testing.
This would require an analysis of proposed workarounds and decomposing to a smaller pieces of work that we can do iteratively.
I expect this work to start on upcoming fall 2023, closer to October.

@caalador caalador moved this to 🪵Product backlog in Vaadin Flow ongoing work (Vaadin 10+) Sep 21, 2023
@mshabarov mshabarov moved this to 🔖 Normal Priority (P2) in Vaadin Flow bugs & maintenance (Vaadin 10+) Sep 27, 2023
@mshabarov mshabarov moved this from 🪵Product backlog to 🟢Ready to Go in Vaadin Flow ongoing work (Vaadin 10+) Sep 28, 2023
@mshabarov mshabarov moved this from 🟢Ready to Go to 🪵Product backlog in Vaadin Flow ongoing work (Vaadin 10+) Oct 11, 2023
@mshabarov mshabarov moved this from 🪵Product backlog to 🟢Ready to Go in Vaadin Flow ongoing work (Vaadin 10+) Oct 25, 2023
@mshabarov mshabarov self-assigned this Oct 30, 2023
@mshabarov mshabarov moved this from 🟢Ready to Go to ⚒️ In progress in Vaadin Flow ongoing work (Vaadin 10+) Oct 31, 2023
@mshabarov
Copy link
Contributor

mshabarov commented Nov 28, 2023

It took a while for me to triage this ticket and split into smaller parts, here what I eventually get (in priority order):

Two other issues regarding AtPermitAll annotation and UIUnitTest base class are more general developer experience issue and can be considered later after we fix the above bug.

Vaadin Flow team will work on fixing these issue during normal maintenance one by one, I believe they can be done separately. I'll keep you posted about results.

Thanks again for the workarounds, much helpful!

@mshabarov mshabarov removed their assignment Nov 28, 2023
@mshabarov mshabarov moved this from 🏗 WIP to 🔖 Normal Priority (P2) in Vaadin Flow bugs & maintenance (Vaadin 10+) Nov 28, 2023
@mshabarov mshabarov moved this from ⚒️ In progress to 🪵Product backlog in Vaadin Flow ongoing work (Vaadin 10+) Nov 28, 2023
mcollovati added a commit that referenced this issue Jan 25, 2024
Currently classes are loaded with Class.forName, that uses the classloader from
the calling class. However, this might not work on all environments because of
different class loading setup (e.g in Quarkus testing).
This change uses the current thread context class loader as fallback when
Class.forName is not able to load a class.

Fixes #vaadin/quarkus#139
Part of #1655
mcollovati added a commit that referenced this issue Jan 25, 2024
Currently classes are loaded with Class.forName, that uses the classloader from
the calling class. However, this might not work on all environments because of
different class loading setup (e.g in Quarkus testing).
This change uses the current thread context class loader as fallback when
Class.forName is not able to load a class.

Fixes vaadin/quarkus#139
Part of #1655
mcollovati added a commit that referenced this issue Jan 25, 2024
Currently classes are loaded with Class.forName, that uses the classloader from
the calling class. However, this might not work on all environments because of
different class loading setup (e.g in Quarkus testing).
This change uses the current thread context class loader as fallback when
Class.forName is not able to load a class.

Fixes vaadin/quarkus#139
Part of #1655
mshabarov pushed a commit that referenced this issue Jan 30, 2024
Currently classes are loaded with Class.forName, that uses the classloader from
the calling class. However, this might not work on all environments because of
different class loading setup (e.g in Quarkus testing).
This change uses the current thread context class loader as fallback when
Class.forName is not able to load a class.

Fixes vaadin/quarkus#139
Part of #1655
@mcollovati
Copy link
Contributor

mcollovati commented Feb 1, 2024

One issue with running UI Unit Tests with @QuarkusTest is that the HTTP server is started even if it should not be needed.

Quarkus 3.2 introduced the @QuarkusComponentTest annotation for testing with CDI (https://quarkus.io/blog/quarkus-component-test/). Unfortunately, this doesn't seem to play well with UI Unit testing, since a lot of manual setup is required.

@ErrorProne in the provided example repository, the UI Unit tests are also starting the HTTP server. Did you manage in some way to prevent running the whole application and just focus on specific components?

mcollovati added a commit that referenced this issue Feb 2, 2024
Adds a base class for UI Unit testing on Quarkus.
Provides mocks for QuarkusVaadinServlet and QuarkusVaadinServletService
integrated with Quarkus CDI.

Fixes vaadin/quarkus#140
Part of #1655
mcollovati added a commit that referenced this issue Feb 5, 2024
This change allows to write UI Unit tests that profits of Quarkus security
testing annotation that can be applied to test methods and test classes to
control the security context that the test is run with for mock user.

Fixes vaadin/quarkus#141
Part of #1655
mcollovati added a commit to vaadin/docs that referenced this issue Feb 5, 2024
mshabarov pushed a commit that referenced this issue Feb 9, 2024
Adds a base class for UI Unit testing on Quarkus.
Provides mocks for QuarkusVaadinServlet and QuarkusVaadinServletService
integrated with Quarkus CDI.

Fixes vaadin/quarkus#140
Part of #1655
mcollovati added a commit that referenced this issue Feb 9, 2024
This change allows to write UI Unit tests that profits of Quarkus security
testing annotation that can be applied to test methods and test classes to
control the security context that the test is run with for mock user.

Fixes vaadin/quarkus#141
Part of #1655
mshabarov added a commit that referenced this issue Feb 16, 2024
* feat: add support for Quarkus security in UI Unit tests

This change allows to write UI Unit tests that profits of Quarkus security
testing annotation that can be applied to test methods and test classes to
control the security context that the test is run with for mock user.

Fixes vaadin/quarkus#141
Part of #1655

* Apply suggestions from code review

Co-authored-by: Mikhail Shabarov <[email protected]>

---------

Co-authored-by: Mikhail Shabarov <[email protected]>
russelljtdyer added a commit to vaadin/docs that referenced this issue Mar 5, 2024
* feat: add documentation for Quarkus UI Unit Testing

Fixes #3169
Part of vaadin/quarkus#140
Part of vaadin/quarkus#141
Part of vaadin/testbench#1655

* add since tag

* add simple test to example code

* First full pass at editing.

* Second full pass at editing.

---------

Co-authored-by: Russell J.T. Dyer <[email protected]>
Co-authored-by: Mikhail Shabarov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 🔖 Normal Priority (P2)
Status: 🪵Product backlog
Development

No branches or pull requests

6 participants