vaadin / quarkus

An extension to Quarkus to support Vaadin Flow
Apache License 2.0
28 stars 3 forks source link

Classloading issue with testbench #121

Closed ErrorProne closed 11 months ago

ErrorProne commented 1 year ago

I've a hard time getting pro features to work in quarkus. It looks like UIUnitTests are not able to load project files.

For a simple cases like this

@QuarkusTest
class ReportViewTest extends UIUnitTest {
    @Test
    void removemelater() {
        final ReportView reportView = navigate(ReportView.class);
    }
}

The tests crashes early in Vaadins autodiscover method

java.lang.ClassNotFoundException: mytest.vaadinpoc.ReportView
    at io.quarkus.bootstrap.classloading.QuarkusClassLoader.loadClass(QuarkusClassLoader.java:489)
    at io.quarkus.bootstrap.classloading.QuarkusClassLoader.loadClass(QuarkusClassLoader.java:466)
    at java.base/java.lang.Class.forName0(Native Method)
    at java.base/java.lang.Class.forName(Class.java:375)
    at com.vaadin.testbench.unit.internal.Routes.autoDiscoverViews(Routes.kt:82)

The debugger shows me that the project files are excluded from the debugger which loads the extension (and seems to be used in subsequent calls).

This looks like it can be fixed by changing the Routes.kt:autoDiscoverViews method to this

@JvmOverloads
    fun autoDiscoverViews(vararg packageNames: String? = arrayOf()): Routes = apply {
        val classGraph: ClassGraph = ClassGraph().enableClassInfo()
                .enableAnnotationInfo()
                .acceptPackages(*(packageNames.map { it ?: "" }.toTypedArray()))
        classGraph.scan().use { scanResult: ScanResult ->
            scanResult.getClassesWithAnnotation(Route::class.java.name).mapTo(routes) { info: ClassInfo ->
                ClassLoader.getSystemClassLoader().loadClass(info.name).asSubclass(Component::class.java) <-- !!! breaks here
            }
            scanResult.getClassesImplementing(HasErrorParameter::class.java.name).mapTo(errorRoutes) { info: ClassInfo ->
                ClassLoader.getSystemClassLoader().loadClass(info.name).asSubclass(HasErrorParameter::class.java)
            }
        }

        cleanupErrorRoutes()

        println("Auto-discovered views: $this")
    }

So basically replacing the Class.forName calls. I'm however not aware of the side effects this might have. A very wild guess is that quarkus loads the extensions with a separate classloader to isolate the extension and prevent name conflicts. Maybe there is something that can be changed in the extension so that a different classloader is used which is capable of loading project files without touching the code of the testbench.

Environment: Quarkus 3.0.2 Vaadin 24.+ JDK 17 (corretto)

Links: Vaadin discord discussion: https://discord.com/channels/732335336448852018/1105486407331479583 Question in quarkus github: https://github.com/quarkusio/quarkus/discussions/33240

mcollovati commented 1 year ago

As a workaround, if the views do not need dependency injection, the test can be executed as a "simple" JUnit test, without the usage of the Quarkus test extension.

benjaminrau commented 11 months ago

Hi there.

Our team is trying to get UI Unit tests running while having CDI available for dependency injection. No chance yet.

Tried to workaround the issues Finn pointed out but ending up in the routes not beeing registered.

To me the severity is definitely higher since any view which is not trivial will rely on repositories, eventbus or other services.

I even spent another hour searching for anybody out there who has published an example @QuarkusTest which has CDI + Vaadin UIUnitTest support. Didnt find any.

So to me this is a blocker for any Vaadin implementations on Quarkus.

Anyway thanks for all your afforts.

Cheers Benjamin

mcollovati commented 11 months ago

Another potential workaround, but it will probably be not enough as there are a few places where UI Unit testing is using Class.forName:

@QuarkusTest
class ReportViewTest extends UIUnitTest {

    @Override
    @BeforeEach
    protected void initVaadinEnvironment() {
        Routes routes = new Routes().merge(new Routes(Set.of(ReportView.class), Set.of(), true));
        MockVaadin.setup(routes, MockedUI::new, Set.of(TestInstantiatorFactory.class));
    }

    @Test
    void testMe() {
        final ReportView reportView = navigate(ReportView.class);
        test(reportView.button).click();

        Assertions.assertEquals("ABCD", test(reportView.input).getValue());
        System.out.println(reportView);
    }

    public static class TestInstantiatorFactory implements InstantiatorFactory {
        @Override
        public Instantiator createInstantitor(VaadinService service) {
            BeanManager beanManager = CDI.current().getBeanManager();
            return new QuarkusInstantiator(new DefaultInstantiator(service),
                    beanManager);
        }
    }
}

And here the application code under test

@ApplicationScoped
public class MyService {

    public String work() {
        return "ABCD";
    }
}

@Route("xxx")
public class ReportView extends Div {

    final Input input;
    final NativeButton button;

    public ReportView(MyService myService) {
        input = new Input();
        button = new NativeButton("OK", ev -> input.setValue(myService.work()));
        add(input, button);
    }
}

I hope this could at least let you set up some basic tests, until a proper integration will be ready

ErrorProne commented 11 months ago

Another potential workaround, but it will probably be not enough as there are a few places where UI Unit testing is using Class.forName:

Interesting. I'll try it out ASAP and report back

benjaminrau commented 11 months ago

Thanks for your help. Unfortunately BaseUIUnitTest.getCurrentView throws a IndexOutOfBoundsExceptions for verifyAndGetUI().getInternals().getActiveRouterTargetsChain().get(0).

Ive taken a similar approach few days ago and experience the same exception.

The verifyAndGetUI().getInternals().getActiveRouterTargetsChain() is empty. getRouter().getRegistry().getRegisteredRoutes() returns the added route. verifyAndGetUI().getInternals().getActiveViewLocation().getPath() is an empty string (root?).

Also tried to get some inspiration from the following implementation - but it lead me to a solution yet: https://github.com/mvysny/vaadin-quarkus/blob/main/src/test/java/org/acme/servlet/MockServlet.java

mcollovati commented 11 months ago

@benjaminrau in my example, I have no issues calling getCurrentView() after navigate(...) Can you provide a sample code that reproduces your issue?

ErrorProne commented 11 months ago

Thanks for your help. Unfortunately BaseUIUnitTest.getCurrentView throws a IndexOutOfBoundsExceptions for verifyAndGetUI().getInternals().getActiveRouterTargetsChain().get(0).

Ive taken a similar approach few days ago and experience the same exception.

The verifyAndGetUI().getInternals().getActiveRouterTargetsChain() is empty. getRouter().getRegistry().getRegisteredRoutes() returns the added route. verifyAndGetUI().getInternals().getActiveViewLocation().getPath() is an empty string (root?).

Also tried to get some inspiration from the following implementation - but it lead me to a solution yet: https://github.com/mvysny/vaadin-quarkus/blob/main/src/test/java/org/acme/servlet/MockServlet.java

That particular problem has something to do with the authentication. The exception is somewhat misleading. But basically the test ist not authorized which leads to the point that no view is loaded and that is why getActiveRouterTargetsChain() returns an empty list. I'm currently debugging why quarkus @TestSecurity is not working in this case and will report back =)

ErrorProne commented 11 months ago

@mcollovati please correct me if I'm wrong. Besides the spring integration there is no actual implementation for auth in the mockservlets? At least intellijs usage search tells me that the principal in requests is set only in the SpringServlet.

My first try was to use the same mechanic and implement a MockRequestCustomizer, but this failed. It looks like this

service.context.getAttribute(Lookup::class.java)
                .lookup(MockRequestCustomizer::class.java)?.apply(mockRequest)

in the MockVaadin.kt only looks for customizers in the vaadin package, which is very sad since this would have been an awesome way to do things in tests.

In my second try I overloaded the setMockRequestFactory to set a new principal and userInRole method based on Quarkus SecurityIdentity. I'll now try to clean everything up and post the result here later.

mcollovati commented 11 months ago

@ErrorProne you're right. Currently security support has (and MockRequestCustomizer) has been implemented only for Spring. It would be worth it to create an issue on Testbench repository to request that feature.

Below you a find a workaround to plug in the MockRequestCustomizer to inject security stuff


@QuarkusTest
class MyTest extends UIUnitTest {

    @Override
    @BeforeEach
    protected void initVaadinEnvironment() {
        Routes routes = new Routes().merge(new Routes(Set.of(ReportView.class), Set.of(), true));
        MockVaadin.setup(routes, MockedUI::new, Set.of(TestLookupInitializer.class, TestInstantiatorFactory.class));
    }

    public static class TestInstantiatorFactory implements InstantiatorFactory {

        @Override
        public Instantiator createInstantitor(VaadinService service) {
            BeanManager beanManager = CDI.current().getBeanManager();
            return new QuarkusInstantiator(new DefaultInstantiator(service),
                    beanManager);
        }
    }

    public static class TestLookupInitializer extends LookupInitializer {
        @Override
        public void initialize(VaadinContext context, Map<Class<?>, Collection<Class<?>>> services, VaadinApplicationInitializationBootstrap bootstrap) throws ServletException {
            ensureService(services, MockRequestCustomizer.class, QuarkusSecurityCustomizer.class);
            super.initialize(context, services, bootstrap);
        }
    }

    public class QuarkusSecurityCustomizer implements MockRequestCustomizer {

        @Override
        public void apply(@NotNull MockRequest mockRequest) {
            // security customizations, such as
            // mockRequest.setUserPrincipalInt(...);
            // mockRequest.setUserInRole(...);
        }
    }

}
ErrorProne commented 11 months ago

Thanks, with my tinkering I've implemented it this way:

MockVaadin.INSTANCE.setMockRequestFactory((MockHttpSession session) -> {
            MockRequest mockRequest = new MockRequest(session);
            mockRequest.setUserPrincipalInt(new PrincipalWithRoles(securityIdentity));
            mockRequest.setUserInRole(PutawayOrderListViewTest::isGranted);
            return mockRequest;
        });

Is there a downside to this approach, besides the obvious that I would have to keep track with the vaadin changes, or is the one with the LookupInitializer just the preferred way? I've done it this way pretty much of the comment I found on the mockRequestFactory

    /**
     * Creates [MockRequest]; override if you need to return a class that extends [MockRequest]
     * and modifies its behavior.
     */
    var mockRequestFactory: (MockHttpSession) -> MockRequest = { MockRequest(it) }

Also there has to be some quarkus specific stuff since QuarkusPrincipal has no roles like the one from spring does. But the method in the UserInRole takes only the Principal and target role.

    private static class PrincipalWithRoles extends QuarkusPrincipal {
        @Getter
        Set<String> roles;

        public PrincipalWithRoles(SecurityIdentity identity) {
            super(identity.getPrincipal().getName());
            this.roles = identity.getRoles();
        }
    }

    /**
     * Very simple check to test if a user has a certain role
     */
    private static boolean isGranted(Principal principal, String role) {
        if (!(principal instanceof PrincipalWithRoles)) {
            return false;
        }

        return ((PrincipalWithRoles)principal).getRoles().contains(role);
    }

Should (something like) this be part of the testbench implementation? I would create a ticket in that repository than.

mcollovati commented 11 months ago

Your solution seems safe to me. Using MockRequestCustomizer would probably be a cleaner approach, since it allows hooking into the request without replacing the core components, but unfortunately, it works out-of-the-box only for spring at the moment.

For the roles check, maybe the setUserInRole can be implemented based on SecurityIdentity::hasRole (not tested, just an idea)

mockRequest.setUserPrincipalInt(sec.getPrincipal());
mockRequest.setUserInRole( (p,r) -> {
    return !sec.isAnonymous() && sec.getPrincipal().equals(p) && sec.hasRole(r);
    // or maybe just sec.hasRole(r)?
});

Having support for Quarkus in testing would be a nice feature for Testbench UI Unit Test, so creating a ticket may be worth it.

ErrorProne commented 11 months ago

Sounds like an idea. I'll look into it. Right now it feels like a very hacky implementation anyway and we have to find a way to combine the UiUnitTest-Class and our normal QuarkusIsolatedTest class and clean everything up. But that should be the easier part.

Having support for Quarkus in testing would be a nice feature for Testbench UI Unit Test, so creating a ticket may be worth it.

Most definitly, I'm currently writing one. When we switched to Vaadin for our current project we pro-actively talked to some Vaadin representatives very explicitly stating that we intend to use quarkus and do a lot of testing. Which has been told to us is no problem. To say our Vaadin experience got quite a bump when we realized that the pro features we pay and asked for do not work out of the box, or are even document in that way, would be an understatement.

@mcollovati thank you very much for your support and hints, they where very helpful and put us into the right direction.

mcollovati commented 11 months ago

I'll close this issue, so the discussion can continue in the testbench repository, on vaadin/testbench#1655 issue