[Feature] JUnit5 integration

[yury-s]

To provide decent support of JUnit in codegen (as requested in #1039), we need to first implement basic Playwright fixtures similar to other languages. This came up in the discussion of #1366 and whether/how we can integrate https://github.com/uchagani/junit-playwright into Playwright. Below are some of the requirements we'll have for such integration and some comments regarding how it compares to the existing junit-playwright.

The execution model should be similar to the default execution model of Node.js Playwright. I.e. it should be possible to

• configure browser options per test file, so that an instance of the browser is reused by all tests in the same file
• do not parallel tests in the same file, only run in parallel tests in different classes (files), i.e. in terms of junit config:
  junit.jupiter.execution.parallel.enabled = true
  junit.jupiter.execution.parallel.mode.default = same_thread
  junit.jupiter.execution.parallel.mode.classes.default = concurrent

Builtin fixtures should create Playwright, Browser, BrowserContext and Page. Their lifetimes should match those in Node.js version, i.e.

• new context and page are created before each test
• for each test, same instance of page and context is used within the test, @beforeeach and @afterEach hooks
• page and context are closed right after last @afterEach hook finished for the test

Browser & playwright lifetimes should be bound to the thread & browser configuration. Playwright instance most likely should be thread local and can be reused by all tests running on that thread. We should try to reuse the same browser instance for running more tests that require the browser launch options. Currently, playwright internal test suite creates new browser per test class which seems to be good enough.

To keep things simple in the beginning we'd like to replace PlaywrightBrowserConfig with a configurable BrowserFactory, so that we don't have to think about config format and let the user create custom browser themselves. Eventually, we'll likely want to load the config from a json/xml/.properties file. We'll likely need to experiment with a few approaches here, to strike a balance.

[yury-s]

Looking at junit-playwright it appears that it currently closes Playwright after each test which is too aggressive, we'll need to align that with the requirements above.

[uchagani]

do not parallel tests in the same file, only run in parallel tests in different classes (files)

I don't think we can enforce this. It would be up to the end user how they configure it. I think all we can do is handle both situations (which is what junit-playwright currently does). However, we can make it so the same Playwright and Browser objects are used for all tests in the file regardless of if they are running serially or in parallel.

If I understand the requirements correctly the lifecycles should be:

Playwright - Per class
Browser - Per class
BrowserContext - Per test
Page - Per test

Is my understanding correct?

[yury-s]

I don't think we can enforce this. It would be up to the end user how they configure it.

I didn't mean enforcing it, but rather provide implementation that only supports the execution model where tests from the same class are not expected to run in parallel. If someone intentionally tries to circumvent this, they are on their own. We can throw or print a warning in some obvious cases where we detect this.

I think all we can do is handle both situations (which is what junit-playwright currently does).

We can also support second mode with parallelism at method level but I'd hold off from it for now to keep things simple.

However, we can make it so the same Playwright and Browser objects are used for all tests in the file regardless of if they are running serially or in parallel.

We cannot do this as it would mean concurrent access to the same Playwright/Browser instance from more than one thread which is not supported by Playwright java.

If I understand the requirements correctly the lifecycles should be:

Playwright - Per class Browser - Per class BrowserContext - Per test Page - Per test

Is my understanding correct?

BrowserContext - Per test Page - Per test

Yes

Playwright - Per class

It can be just thread local and reused by all tests running on that thread which can include multiple classes.

Browser - Per class

We can start with this for simplicity. Later it can be optimized to reuse the browser between more classes running on the same thread and requiring same browser configuration. E.g. if there are 3 test classes that require Chromium headless with default parameters, there is no need to close the browser between them. But if say one of the 3 classes contains tests that need to run in headed mode, we'll need to close browser and launch a new one for tests in that class.

[uchagani]

@yury-s I've started working on this and I think it will be fairly easy to support same_thread and concurrent right away. I've already got a working sample.

Your idea of using ThreadLocal makes things much simpler. When running with same_thread, everything in a class is run on the same thread and serially so ThreadLocal works great for everything. When running with concurrent, we would want each test to have its own playwright/browser/etc objects so that too works great with ThreadLocal also because each test and it's lifecycle methods run on the same thread. we just close them after each test instead of in an AfterAll hook.

we'd like to replace PlaywrightBrowserConfig with a configurable BrowserFactory, so that we don't have to think about config format and let the user create custom browser themselves.

Do you have a suggestion of what this should look like?

[yury-s]

Do you have a suggestion of what this should look like?

At first glance, something like

@UseBrowserFactory(CustomBrowserFactory.class)
@UseContextFactory(CustomContextFactory.class)

public class CustomBrowserFactory implements BrowserFactory {
    @Override
    public Browser newBrowser(Playwright playwright) {
        return playwright.chromium().launch(new BrowserType.LaunchOptions().setChannel("msedge"));
    }
}

public class CustomContextFactory implements ContextFactory {
    @Override
    public Browser newContext(Browser browser) {
        return browser.newContext(new Browser.NewContextOptions().setViewportSize(1600, 1200).setLocale("ar‑AR"));
    }
}

And if no annotation is specified, use default ones. The default could create just chromium and a context with default parameters. We discussed this offline and feel like it'd be better than introducing another option for the browser/context configs when there are existing classes for that. WDYT?

[uchagani]

I like it. Do you think it would be worth it to make 1 interface called PlaywrightFactory and have all the methods in there with just 1 annotation?

Also, I came across one downside of supporting concurrent access: BeforeAll and AfterAll hooks run in separate threads than the tests themselves so we couldn't inject a playwright fixture into the test in those hooks and then reuse the same one in the test. We could make it so an error is thrown if BeforeAll/AfterAll hooks are used with concurrent access using the playwright extension. I think this is a good compromise for now. WDYT?

[yury-s]

I like it. Do you think it would be worth it to make 1 interface called PlaywrightFactory and have all the methods in there with just 1 annotation?

I feel like it'd be easier to keep them separate but it's hard to tell without seeing it. We can toy with that and change to the one that we like more.

Also, I came across one downside of supporting concurrent access: BeforeAll and AfterAll hooks run in separate threads than the tests themselves so we couldn't inject a playwright fixture into the test in those hooks and then reuse the same one in the test. We could make it so an error is thrown if BeforeAll/AfterAll hooks are used with concurrent access using the playwright extension. I think this is a good compromise for now. WDYT?

That's a good catch. This is only a problem if you want to run each test in parallel with the others. In that case, we can take same approach as in node.js and pretend that each of the tests runs in its own JVM process and run BeforeAll/AfterAll on each thread before/after corresponding tests. Doe that make sense? This is another argument in favor not running tests from same class in parallel, in that case we don't have the problem. Behavior would still differ from node.js where we rerun beforeAll (in a new test worker process) if any of the tests in the file fail.

[uchagani]

In that case, we can take same approach as in node.js and pretend that each of the tests runs in its own JVM process and run BeforeAll/AfterAll on each thread before/after corresponding tests. Doe that make sense?

We'll need to talk about this some more because I'm not sure I understand. I'm not managing threads myself and letting JUnit handle all of that. Maybe there is a way to do this without having the BeforeAll/AfterAll repeat before each test. For now I won't worry about it.

This is another argument in favor not running tests from same class in parallel

Definitely understand. I guess I'm being a bit selfish here because I use full parallel a lot so I don't have to worry about splitting up test files just to get a faster run.

I don't think we will make the 1.38 release. I think 1.39 would be doable assuming everything goes smoothly in reviews.

[yury-s]

We'll need to talk about this some more because I'm not sure I understand. I'm not managing threads myself and letting JUnit handle all of that. Maybe there is a way to do this without having the BeforeAll/AfterAll repeat before each test. For now I won't worry about it.

Yeah, this needs more thought. I'll bring it up with the team next week, maybe someone has a better idea.

I don't think we will make the 1.38 release. I think 1.39 would be doable assuming everything goes smoothly in reviews.

No rush here, 1.39 is totally fine. It's more important we do it right and don't repeat the regrets from other language ports.

[uchagani]

@yury-s is it necessary to call BrowserContext.close() and Browser.close() or can we just call Playwright.close() when we are done with the test/test class since closing playwright will close everything else too? In the docs it says

This is similar to force quitting the browser. Therefore, you should call browserContext.close() on any BrowserContext's you explicitly created earlier with browser.newContext() before calling browser.close()

but what's the downside?

[uchagani]

@yury-s question: Currently in playwright-java, is a user allowed to create multiple browsers with the same Playwright process? For example will this work with the sync api of playwright-java?

Playwright playwright = Playwright.create();
Browser chrome = playwright.chromium.launch();
Browser firefox = playwright.firefox.launch();

// use both browsers one at a time.

Or should they close one and then launch the other?

[yury-s]

@yury-s is it necessary to call BrowserContext.close() and Browser.close() or can we just call Playwright.close() when we are done with the test/test class since closing playwright will close everything else too? In the docs it says

If you are going to close playwright right away then technically it is not necessary. But at the very least as a good code practice let's keep context.close(), browser.close() matching corresponding creation of the classes. Also, since we want to reuse browser instance between the tests in the same class, it is important to close the context between tests.

@yury-s question: Currently in playwright-java, is a user allowed to create multiple browsers with the same Playwright process? For example will this work with the sync api of playwright-java?

It is perfectly fine to create multiple browsers with the same playwright instance, this is no different than node.js. It is very difficult though to use such browsers simultaneously in java with synchronous API, in node.js promises enable users naturally mix calls to different browsers.

Or should they close one and then launch the other?

No. They can coexist without any issues.

[uchagani]

@yury-s, if a user requests a BrowserContext or Page in a class-level hook (BeforeAll/AfterAll), what should be done with those objects? Should we provide one and then close them before the first test starts since we want each test to have its own BrowserContext and Page? If we create one at the class level, it doesn't really belong to any particular test.

[uchagani]

@yury-s, since BrowserContext is scoped to the test, do we want the user to be able to configure different BrowserContextFactory for each test? In other words, do we want the user to be able to do:

@UseBrowserFactory
@UseBrowserContextFactory //use default for all tests except ones defined at method level
public class FooTests {

  void test1(BrowserContext browserContext) {
    // test will use the BrowserContext defined at the class level.
  }

  @UseBrowserContextFactory(CustomBrowserContextFactory.class) 
  void test2(BrowserContext browserContext) {
    // test will use the BrowserContext defined at the method level
  }

}

[yury-s]

@yury-s, since BrowserContext is scoped to the test, do we want the user to be able to configure different BrowserContextFactory for each test? In other words, do we want the user to be able to do:

I'd start with class level configuration only, we can support method level annotations later. In the default setup all tests in the same class use the same configuration which means that all Playwright/Browser/Context/Page factories are the same. Also I'd imply default Playwright/Browser factories if they are not specified explicitly and only UseBrowserContext annotation is present.

[uchagani]

I'd imply default Playwright/Browser factories if they are not specified explicitly and only UseBrowserContext annotation is present.

@yury-s couple points of clarification here:

1. We don't currently have a PlaywrightFactory. Should we add one?
2. Since we want to imply default configurations for the factories, have you given any thought to having a single annotation? I think it would simplify the API for the user and we can use some of the same naming conventions as node playwright. For example:

@Use //use all default factories

@Use(browserFactory = FirefoxBrowserFactory.class) // use custom browser but default browser context

@Use(browserContextFactory = CustomBrowserContextFactory.class) // use default browser but custom browser context

@Use(browserFactory = FirefoxBrowserFactory.class, browserContextFactory = CustomBrowserContextFactory.class) // use custom everything

[yury-s]

• We don't currently have a PlaywrightFactory. Should we add one?

Playwright.create() may take non-default options and if someone wants to override that we need to have a factory, otherwise they are bound to the default one and won't be able to use the fixtures.

• Since we want to imply default configurations for the factories, have you given any thought to having a single annotation? I think it would simplify the API for the user and we can use some of the same naming conventions as node playwright. For example:

I like this. We just need a playwright-specific name for the annotation, something like @UsePlaywrightFixtures(...) and then it would populate Playwright, Browser, BrowserContext, Page parameters. The user could override some of the factories as you described (@UsePlaywrightFixtures(browserFactory = FirefoxBrowserFactory.class, browserContextFactory = CustomBrowserContextFactory.class)) or just use default ones. And if we want to use per method/parameter annotations we should be able to reconcile the two in the future.

[yury-s]

@uchagani Let's hold off migration of the tests to the new fixtures for now. We just reviewed the APIs with the team and think that if we don't support config annotations right away, it may be difficult to get clients to use it in the future. We'd like to explore the idea of config annotations and see if it would make sense to introduce them from the very beginning. The main motivation is that in order to provide good ergonomics for things like tracing, we need a better control over the options and how browsers/contexts created. I understand that the back and forth movement may cause frustration, but we'd rather do a few iterations before the APIs are released to figure out which is the best for the clients. Hope you understand.

[uchagani]

@yury-s I understand. Can you please provide some requirements around what you would want for the config annotations?

[yury-s]

We were thinking about something like Config class in this change, which would combine playwright/context/browser/apirequest options as well as things like browserName and let the client specify one of the config constants that they may have in the project to run the test class. We may well change this while iterating on this, but this is something that seems to be a good starting point.

We'd love to also include trace as an option there, as tracing in particular is tricky to configure and collect after test runs. The challenge with such options is that we need to make sure it integrates with the JUnit's report. I.e. if we write a trace file it will be referenced from the corresponding report and can be viewed later when the report is uploaded to the CI's dashboard. I don't know off the top of my head what it would take and what approaches JUnit, surefire plugin etc use for adding attachments, this will need to be investigated. We can probably omit it in the initial version.

The change hacks it on top of the existing factories but if we want to support tracing out of the box we'll likely need full control over the lifecycle of browser/context/page and what parameters are passed to their constructors. This will likely rule out extensibility with factories as they may try to pass conflicting options.

I hope this clarifies things a bit.

[uchagani]

@yury-s a few questions and comments regarding the changes:

typo: contextOption

what is the typo? Do you mean it should be contextOptions (plural)?

create one Playwright instance per thread, close it after all tests on the thread finished.

The event you recommended is for custom launchers and we're not creating that. I'm not sure if there is a way to know when a thread is finished since I did a quick test and threads are reused (as you said). I do have an idea of how we can keep all playwright instances open until all tests are finished but this will be after the entire test run is complete (which is what I think you meant when you recommended that launcher event). I'll get started on that.

add device option

I believe you did this already in #1472 ?

[yury-s]

@yury-s a few questions and comments regarding the changes:

typo: contextOption

what is the typo? Do you mean it should be contextOptions (plural)?

Yes. @jfgreffier already fixed it in #1486

create one Playwright instance per thread, close it after all tests on the thread finished.

The event you recommended is for custom launchers and we're not creating that. I'm not sure if there is a way to know when a thread is finished since I did a quick test and threads are reused (as you said). I do have an idea of how we can keep all playwright instances open until all tests are finished but this will be after the entire test run is complete (which is what I think you meant when you recommended that launcher event). I'll get started on that.

Good point about launchers. Closing playwrights after all tests finish sounds good too, let's try that.

add device option

I believe you did this already in #1472 ?

Yes. I thought about generating constants with the device names, but not sure yet.

[uchagani]

open question: how can we support running same test with different browsers, currently we just pass the browser name as an environment variable and launch mvn test for each browser. Ideally, we'd have a way to parametrize tests with the browser/device names, something along these lines.

@yury-s, I think we may be able to make use of JUnit 5's Test Templates https://www.baeldung.com/junit5-test-templates. Maybe in a follow up release I can help with this too.

[uchagani]

add ingoreHTTPSErrors option

@yury-s should this top-level option apply to both the BrowserContextOptions and the RequestOptions since they both have a setIgnoreHTTPSErrors?

[yury-s]

@yury-s, I think we may be able to make use of JUnit 5's Test Templates https://www.baeldung.com/junit5-test-templates. Maybe in a follow up release I can help with this too.

If it fits our use cases then why not.

add ingoreHTTPSErrors option

@yury-s should this top-level option apply to both the BrowserContextOptions and the RequestOptions since they both have a setIgnoreHTTPSErrors?

Yes, I'd apply it to both. For more granular control the user could still set context specific options.

[jfgreffier]

I'm not sure everything will be ready for 1.42 but here are docs and codegen PR

microsoft/playwright#29406 docs
microsoft/playwright#29424 codegen

[yury-s]

We discussed wether we should try and release junit support for java in 1.42 or postpone it and decided to release it as an experiment. We debated if it would mean putting the classes under .experimental.junit package or just mentioning in javadocs and in playwright.dev that the support is experimental and think that the latter would be preferred. @uchagani, @jfgreffier do you think mentioning experimental in the docs would be sufficient?

We still need to add some javadocs to UsePlaywright and OptionsFactory, so that people could get some help within IDEs.

[yury-s]

I'm not sure everything will be ready for 1.42 but here are docs and codegen PR

microsoft/playwright#29406 docs microsoft/playwright#29424 codegen

@jfgreffier I've just merged the codegen PR. For the docs, let's introduce a new page for JUnit integration and have a prominent marker at the top of it that it is experimental for now. This way we can keep all existing docs as is and add links to the new JUnit guide to let more adventurous people try it while leaving us some leeway to change the API if needed.

[yury-s]

@jfgreffier thanks for writing the doc for playwright.dev, very god one, I approved the PR.

@uchagani @jfgreffier do you want to take a stab at adding javadocs to the fixture classes? If not, I'll work on it in a week. I'd also like to try and switch some of our tests to the fixtures to get a sense of the API, but otherwise I think javadocs is the only remaining bit before we can release it. We are planning to release 1.42 java port in a week, including the junit integration.

[uchagani]

I probably won't have time to get to the javadocs but have already started converting some of the existing tests over to using Fixtures. as I get a logical grouping done, i'll submit PRs for them.

[jfgreffier]

I'll skip the javadocs

[yury-s]

Experimental junit fixtures have been released in 1.42.0, great job folks!

Closing this issue, let's use separate ones for further improvements.