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.

Sign up for GitHub

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

junit: allow user to reset Browser after each test #1692

Closed
wants to merge 2 commits into from
Closed

junit: allow user to reset Browser after each test #1692

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f380176
junit: allow user to reset Browser after each test
uchagani Oct 27, 2024
c9c2f86
Handle null pointer exception
uchagani Oct 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions playwright/src/main/java/com/microsoft/playwright/impl/junit/BrowserContextExtension.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,13 @@ public static BrowserContext getOrCreateBrowserContext(ExtensionContext extensio
@Override
public void testSuccessful(ExtensionContext extensionContext) {
saveTraceWhenOn(extensionContext);
closeBrowserContext();
cleanup(OptionsExtension.getOptions(extensionContext));
}

@Override
public void testAborted(ExtensionContext extensionContext, Throwable cause) {
saveTraceWhenOn(extensionContext);
closeBrowserContext();
cleanup(OptionsExtension.getOptions(extensionContext));
}

@Override
Expand All @@ -90,7 +90,14 @@ public void testFailed(ExtensionContext extensionContext, Throwable cause) {
if (shouldRecordTrace(options)) {
saveTrace(extensionContext);
}
cleanup(options);
}

private void cleanup(Options options) {
closeBrowserContext();
if(options.closeBrowserAfterEachTest != null && options.closeBrowserAfterEachTest) {
BrowserExtension.closeBrowser();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior of closing the browser (rather than just the context) after each test is very specific to BrowserStack implementation and they could solve it better by providing an API for explicitly marking test start/end instead of binding it to the lifetime of the browser connection.

I'd rather the clients, that need to close browsers after each test, implemented their own fixture and just called browser.close() after each test from the fixture. We can add onDisconnect handler to all browser instances that are created via Playwright fixture and clean-up thread local cache when the event is fired. It would also cover the case when the user calls browser.close() in the middle of a test too, so it's something that would make sense to do in any case.

Do you think such approach would make more sense or you'd still prefer to have the option?

}
}

private static void saveTraceWhenOn(ExtensionContext extensionContext) {
Expand Down
10 changes: 10 additions & 0 deletions playwright/src/main/java/com/microsoft/playwright/impl/junit/BrowserExtension.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,16 @@ public Object resolveParameter(ParameterContext parameterContext, ExtensionConte
return getOrCreateBrowser(extensionContext);
}

static void closeBrowser() {
Browser browser = threadLocalBrowser.get();
if(browser == null) {
return;
}

browser.close();
threadLocalBrowser.remove();
}

/**
* Returns the Browser that belongs to the current test. Will be created if it doesn't already exist.
* <strong>NOTE:</strong> this method is subject to change.
Expand Down
8 changes: 8 additions & 0 deletions playwright/src/main/java/com/microsoft/playwright/junit/Options.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,21 @@ public class Options {
public Path outputDir;
// When to record traces. Default is OFF.
public Trace trace = Trace.OFF;
// This should only be used if a new Browser object is needed for each test.
// Usually only necessary when using a cloud testing service
public Boolean closeBrowserAfterEachTest;

public enum Trace {
OFF,
ON,
RETAIN_ON_FAILURE;
}

public Options setCloseBrowserAfterEachTest(Boolean closeBrowser) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we proceed with this option, let's have a test for it.

this.closeBrowserAfterEachTest = closeBrowser;
return this;
}

public Options setTrace(Trace trace) {
this.trace = trace;
return this;
Expand Down
Loading