[Feature]: JUnit: Allow the user to control the Browser object's lifecycle

[uchagani]

🚀 Feature Request

Add an option that will reset the Browser object after each test so cloud services that use BrowserType.connect can work properly. Example: https://www.browserstack.com/docs/automate/playwright/playwright-capabilities#Java

Example

public class Options {
  // existing options

  public Boolean closeBrowserAfterEachTest;

  public Options setCloseBrowserAfterEachTest(Boolean closeBrowser) {
    this.closeBrowserAfterEachTest = closeBrowser;
    return this;
  }

}

Inside of the existing TestWatcher methods, we can check if the user wants to close the browser after each test and do so. Once the next test starts, the Browser will be re-created by the fixture using the existing options. This will allow a new test session to start in BrowserStack.

  @Override
  public void testSuccessful(ExtensionContext extensionContext) {
    saveTraceWhenOn(extensionContext);
    closeBrowserContext();
    Options options = OptionsExtension.getOptions(extensionContext); 
    if(options.closeBrowserAfterEachTest) {
      BrowserExtension.closeBrowser();
    }
  }

Motivation

Cloud testing services like BrowserStack launch new sessions using BrowserType.connect. Each call to connect starts a new test session.

This doesn't work properly with current JUnit fixtures because the Browser object is re-used per thread and so all tests that run on the same thread use the same test session in BrowserStack. This becomes one giant test according to BrowserStack.

My suggestion would allow the user to decide when to re-use the browser, which works great locally, but not so well when using cloud testing services.

[yury-s]

Not to say that I'm completely against the idea, but it sounds like a workaround for a limitation of a particular cloud service. Do you know why they require resetting the connection for each test rather than reusing the connection and creating BrowserContext per test as we do? Is it because there is no separate API to mark test begin/end?

[uchagani]

I'm not sure what their reasoning is. My guess is that it's a carry-over from how they implemented their selenium tests. There is an API to mark tests as passed/failed but there is no way to start a new test without establishing a new session.

[yury-s]

I see, thanks for providing more context. I commented on the PR. My preference would be to allow calling browser.close() on the browsers provided by @Playwright fixture and let the clients call that method from their own fixtures if needed. But I'm not strongly opposed to having the option either if there is a good rationale for that.

[uchagani]

@yury-s I'll test out your proposed solution tomorrow and see how it works out.

[uchagani]

@yury-s using onDisconnected works but there are a couple drawbacks:

1. The user cannot call browser.close() from within a normal JUnit AfterEach hook. If they do, all of the trace recording that happens in the test watcher methods will not work because the BrowserContext will be closed. So instead the user has to create their own TestWatcher and call BrowserExtension.getOrCreateBrowser(extensionContext).close(). This works fine.
2. The order of the extension registration will matter if we go this route. We need to have the trace-related methods execute first before the browser is closed. So the user must be careful how they apply the annotations:

works:

@ExtendWith(BrowserStackTestWatcher.class)
@UsePlaywright(SomeOptions.class)
public class MyTests {
}

doesn't work:

@UsePlaywright(SomeOptions.class)
@ExtendWith(BrowserStackTestWatcher.class)
public class MyTests {
}

I think this trade-off is acceptable for now because this is a one off thing for services like BrowserStack. If you agree I can make the changes in my branch.

[yury-s]

• The user cannot call browser.close() from within a normal JUnit AfterEach hook. If they do, all of the trace recording that happens in the test watcher methods will not work because the BrowserContext will be closed. So instead the user has to create their own TestWatcher and call BrowserExtension.getOrCreateBrowser(extensionContext).close(). This works fine.

I believe we should address this. I hope the traces can be saved when BrowserContext.onClose is emitted, that would work for the cases when the context is closed, browser is closed or just test fails/ends (triggering context close). Would that work? At least this is how it is done in Node.

• The order of the extension registration will matter if we go this route. We need to have the trace-related methods execute first before the browser is closed. So the user must be careful how they apply the annotations:

This is counter intuitive and it'd be nice if we could avoid that. I hope we can save traces on context close and avoid this problem, but if we don't, this ordering constrain should be a minor limitation.

[uchagani]

I hope the traces can be saved when BrowserContext.onClose is emitted, that would work for the cases when the context is closed

@yury-s I really like this idea but there is one drawback that I think can be worked around but before I implement I wanted to get your thoughts:

In JUnit, we don't know whether a test passed or failed until the TestWatcher method execute. In the case of a RETAIN_ON_FAILURE trace setting, we will still have to wait for the TestWatcher methods to execute to know whether to save the trace or not, however if the Browser is closed at that point by the user in their AfterEach hook, we won't be able to save the trace.

The workaround I can think of is to save the trace always, and then delete the trace if the trace setting is RETAIN_ON_FAILURE. The flow would be:

1. Start trace as we currently do
2. Configure BrowserContext.onClose to save the trace file if trace setting is ON or RETAIN_ON_FAILURE
3. In the TestWatcher.testPassed method, if the trace setting is RETAIN_ON_FAILURE, delete the saved trace.

This would allow us to avoid having the user register their own TestWatcher to close the browser and would remove the Extension registration order issue. Does this sound like a good compromise?

[yury-s]

Yes, this sounds good to me. In Node.js we also record the trace if it might be needed when the test is done and then delete it if according to the setting and the test's outcome it should not be preserved. I think it'd be nice if we followed the same logic in the Java por.