[Feature] JUnit5 integration

[yury-s]

To provide decent support of JUnit in codegen (as requested in https://github.com/microsoft/playwright-java/issues/1039), we need to first implement basic Playwright fixtures similar to other languages. This came up in the discussion of https://github.com/microsoft/playwright-java/pull/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 this is similar to what i did in BrowserConfig class in junit-playwright. The API of that follows closely to what you have to do when you're configuring playwright except it creates a fluent API around it.

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.

Can you expand on this some more? I thought playwright-java doesn't provide any CI/trace integrations. Currently what i've done in Jenkins is to archive the directory that contains the trace files. We can have full control of the lifecycle of the objects the fixtures creates including saving the trace files always or only on failures. We can have a default directory or one that is provided by the user via the config.

As far as next steps, do you want to revert the PR that was merged for junit-fixtures and i can open a new PR that contains this new config approach? Also, how do you feel about the fluent api approach i took in junit-playwright for allowing the user to configure their options?

[yury-s]

Can you expand on this some more? I thought playwright-java doesn't provide any CI/trace integrations. Currently what i've done in Jenkins is to archive the directory that contains the trace files. We can have full control of the lifecycle of the objects the fixtures creates including saving the trace files always or only on failures. We can have a default directory or one that is provided by the user via the config.

Managing those artifact manually would work, but we were thinking about making it more convenient like in node.js where you can set trace option and let the test runner save the traces etc into test-specific output directories. This logic can be implemented inside fixtures / lifecycle watcher but we need to make sure that the way we store those attachment will work nicely with things like surefire-report-plugin and other report generators. Perhaps you can already configure them to find the attachment on the filesystem as absolute path in the attachments but we need to understand how they work and validate this. Until this is clarified we can suggest regular tracing API.

As far as next steps, do you want to revert the PR that was merged for junit-fixtures and i can open a new PR that contains this new config approach?

Reverting that and implementing Config fixture would probably be the easiest but it's up to you. If you feel it's better to keep it for now before the Config API is defined I'm fine with that too.

Also, how do you feel about the fluent api approach i took in junit-playwright for allowing the user to configure their options?

Yeah, fluent API sounds good, we use it for options in the API so it will be consistent with that.

[uchagani]

@yury-s, regarding trace options, I think we should support off, on, and retain-on-failure. I don't think we should support retries as the retry will happen via maven surefire/failsafe and, from personal experience, this comes with a separate set of challenges (mainly threading issues when surefire is updating test status of a flaky test which is still not fixed and is waiting on some junit updates).

As far as I understand, the JUnit XML format doesn't natively support attachments. The JUnit Attachments plugin basically combines the features of JUnit and Jenkins to do what it needs to do. My thoughts on this is that there are just too many variables to consider. For example, is the user using Surefire or Failsafe or the JUnit Console Launcher or maybe their own custom launcher?

For this integration, my recommendation would be to support the trace options I listed above and provide a way for the user to specify the location where they would like the trace files saved this is what junit-playwright currently does. This will limit the scope of this integration but also allow the user to use the JUnit Attachments plugin with jenkins. We can use the log line option to integrate with that plugin.

Reverting that and implementing Config fixture would probably be the easiest but it's up to you.

Yes I agree, let's revert that and I will submit a new PR with the new Config changes you recommended. That should keep things cleaner and together.

[yury-s]

As far as I understand, the JUnit XML format doesn't natively support attachments. The JUnit Attachments plugin basically combines the features of JUnit and Jenkins to do what it needs to do.

It's true that there is no standard way to add attachment to junit xml reports, but my understanding is that the attachments "extension" is used in many places, e.g. on Azure ADO DevOps pipelines and it'd be nice to ensure that our API is compatible with that.

My thoughts on this is that there are just too many variables to consider. For example, is the user using Surefire or Failsafe or the JUnit Console Launcher or maybe their own custom launcher?

For this integration, my recommendation would be to support the trace options I listed above and provide a way for the user to specify the location where they would like the trace files saved this is what junit-playwright currently does. This will limit the scope of this integration but also allow the user to use the JUnit Attachments plugin with jenkins. We can use the log line option to integrate with that plugin.

My understanding is that there is just a handful of different reporters for junit tests and we could just go over the major ones (junit xml with the attachments, allure?) and see if/how they could embed the traces with the proposed approach. I hope putting them into a predefined directory would work. In node.js we have an output folder per test and we might need something similar in java. Surefire reports solve it by naming the reports like TEST-com.microsoft.playwright.TestBrowser.xml but if we want to support multiple attachments it might make sense to have a directory per test method where all artifacts are stored or maybe there is already an established approach where to write them. My point is the approach with tracing option as enum of on, off, retain-on-failure and an output dir sounds reasonable, but it requires some investigation/experimentation with existing solutions. At the same time, configuring browser type, context options etc is more straightforward and less controversial.

Yes I agree, let's revert that and I will submit a new PR with the new Config changes you recommended. That should keep things cleaner and together.

Sounds good.

[uchagani]

My point is the approach with tracing option as enum of on, off, retain-on-failure and an output dir sounds reasonable, but it requires some investigation/experimentation with existing solutions. At the same time, configuring browser type, context options etc is more straightforward and less controversial.

Makes sense. I'll concentrate on non-trace related options for now then.

[uchagani]

@yury-s wondering if the team was able to review the current solution and if there was any feedback? Also, is there anything else left to do before we can release this?

[yury-s]

@yury-s wondering if the team was able to review the current solution and if there was any feedback?

We'll discuss it in a meeting tomorrow and I'll get back to you.

Also, is there anything else left to do before we can release this?

Off top of my head:

• we need to try and switch our test harness to test dogfood new classes and see if there are any obvious issues
• need some documentation/examples explaining how to use it similar to https://playwright.dev/dotnet/docs/test-runners#nunit. Perhaps @jfgreffier will continue what he was doing in https://github.com/microsoft/playwright/pull/26749.

[jfgreffier]

Yes, I'm fine to do some documentation. I have to catch up on what was done (especially configuration) and find some time

[uchagani]

Awesome thanks!

I can get started on converting the existing tests over as soon as we get feedback from the team.

[yury-s]

We discussed this with the team today and overall the API looks good. Here are some notes:

• public fields in Options. We were debating wether to remove the getters from Options class or to make corresponding fields private. Given that we use public fields and no getters in other option classes in playwright, I'm leaning slightly towards removing getters, but can be convinced otherwise. What are you thoughts on this?

  • [x] change it one way or the other before the release

• Should Options have a more specific name? - no, as we use @UsePlaywright for annotations and options only appear in the implementation classes of the options factories.

• typo: contextOption

  • [x] rename

• We need to support custom testId attribute in the config similar to nodejs.

  • [x] add an option

• storageStatePath/viewportSize as top level Options? - storageStatePath is handy in node.js because it's written in global setup and then read in each test worker. In JUnit there is no global setup so it is of lower value. As for viewport size, in node.js we use device descriptors (by name) instead.

  • [x] remove both

• Add setDevice option, that accepts a name and implies corresponding context options (viewport size, browser name, isMobile etc). We didn't want to expose the hairy devices API for the library, but we can implement it in the fixtures by exposing only the device name in the config and call some implementation methods from playwright.impl if needed. This would be a great improvement over the current experience.

  • [x] add device option

• ingoreHTTPSErrors - this is mentioned quite a few times in our playwright test examples and seems to be used quite often to make it a top level option.

  • [x] add ingoreHTTPSErrors option

• PlaywrightExtension currently closes Playwright when all the tests in a class finish. We would like to keep it running for the lifetime of the thread, rather than closing after each test class. So maybe close it on something like testPlanExecutionFinished event or when the thread finishes running (to accomodate different test execution strategies in JUnit). Eventually we'll want to do the same with the Browser instance and keep reusing it as long as the next test has the same launch params. Maybe even have a browser pool.

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

• 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.

• trace: automatic tracing support with an option is something for the future iterations, big topic on its own.

I think most of the new options above are non-blocking as we can add them later without breaking backwards compatibility. But the rest would be nice to address before releasing.