-
Notifications
You must be signed in to change notification settings - Fork 255
WIP: config fixture #1398
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.
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
WIP: config fixture #1398
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| package com.microsoft.playwright.junit; | ||
|
|
||
| import com.microsoft.playwright.APIRequest; | ||
| import com.microsoft.playwright.Browser; | ||
| import com.microsoft.playwright.BrowserType; | ||
|
|
||
| import java.nio.file.Path; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| public class Config { | ||
| private BrowserType.LaunchOptions launchOptions = new BrowserType.LaunchOptions(); | ||
| private Browser.NewContextOptions contextOption = new Browser.NewContextOptions(); | ||
| private APIRequest.NewContextOptions apiRequestOptions = new APIRequest.NewContextOptions(); | ||
| private Map<String, Object> clientValues = new HashMap<>(); | ||
|
|
||
| private String browserName = "chromium"; | ||
|
|
||
| public String browserName() { | ||
| return browserName; | ||
| } | ||
|
|
||
| public void setBrowserName(String browserName) { | ||
| this.browserName = browserName; | ||
| } | ||
|
|
||
| public void setHeadless(boolean headless) { | ||
| launchOptions.setHeadless(headless); | ||
| } | ||
|
|
||
| public void setBaseURL(String url) { | ||
| contextOption.setBaseURL(url); | ||
| } | ||
|
|
||
| public void setStorageStatePath(Path path) { | ||
| contextOption.setStorageStatePath(path); | ||
| } | ||
|
|
||
| public BrowserType.LaunchOptions launchOptions() { | ||
| return launchOptions; | ||
| } | ||
|
|
||
| public Browser.NewContextOptions contextOptions() { | ||
| return contextOption; | ||
| } | ||
|
|
||
| public APIRequest.NewContextOptions apiRequestOptions() { | ||
| return apiRequestOptions; | ||
| } | ||
|
|
||
| public <T> void setClientValue(String name, T value) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this for setting PlaywrightOptions?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea here was to have a storage for the clients to pass some options to their tests. I.e. make the Config available in the tests (e.g. as a method parameter similar to Browser etc) and if the client needs to pass some custom properties they could achieve that by using these client property map.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can make the whole Config object available to the test so I'm not sure if we need this but if you want it I can add it.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's omit these two methods for now. Even if we expose Config object it won't have a getter setter for |
||
| clientValues.put(name, value); | ||
| } | ||
|
|
||
| public <T> T getClientValue(String name) { | ||
| return (T) clientValues.get(name); | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| package com.microsoft.playwright.junit; | ||
|
|
||
| public interface ConfigFactory { | ||
| Config newConfig(); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| package com.microsoft.playwright.junit; | ||
|
|
||
| public class DefaultConfigFactory implements ConfigFactory { | ||
| @Override | ||
| public Config newConfig() { | ||
| return new Config(); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| package com.microsoft.playwright.junit.impl; | ||
|
|
||
| import com.microsoft.playwright.*; | ||
| import com.microsoft.playwright.junit.*; | ||
| import org.junit.jupiter.api.extension.*; | ||
|
|
||
| import java.nio.file.FileSystems; | ||
| import java.nio.file.Path; | ||
|
|
||
| import static com.microsoft.playwright.junit.impl.ExtensionUtils.getUsePlaywrightAnnotation; | ||
|
|
||
| public class ConfigExtension implements AfterAllCallback { | ||
| private static final ThreadLocal<Config> threadLocalConfig = new ThreadLocal<>(); | ||
|
|
||
| @Override | ||
| public void afterAll(ExtensionContext extensionContext) { | ||
| threadLocalConfig.remove(); | ||
| } | ||
|
|
||
| static Config getConfig(ExtensionContext extensionContext) { | ||
| Config config = threadLocalConfig.get(); | ||
| if (config != null) { | ||
| return config; | ||
| } | ||
| UsePlaywright usePlaywrightAnnotation = getUsePlaywrightAnnotation(extensionContext); | ||
| if (!DefaultConfigFactory.class.equals(usePlaywrightAnnotation.configFactory())) { | ||
| try { | ||
| ConfigFactory configFactory = usePlaywrightAnnotation.configFactory().newInstance(); | ||
| config = configFactory.newConfig(); | ||
| threadLocalConfig.set(config); | ||
| } catch (InstantiationException | IllegalAccessException e) { | ||
| throw new PlaywrightException("Failed to create config", e); | ||
| } | ||
| } | ||
| return config; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| package com.microsoft.playwright; | ||
|
|
||
| import com.microsoft.playwright.junit.Config; | ||
| import com.microsoft.playwright.junit.ConfigFactory; | ||
| import com.microsoft.playwright.junit.UsePlaywright; | ||
| import org.junit.jupiter.api.*; | ||
|
|
||
| import java.nio.file.Paths; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
|
|
||
|
|
||
| @UsePlaywright(configFactory = TestFixturesWithCustomConfig.CustomConfigFactory.class) | ||
| public class TestFixturesWithCustomConfig { | ||
| public static class CustomConfigFactory implements ConfigFactory { | ||
| @Override | ||
| public Config newConfig() { | ||
| Config config = new Config(); | ||
| config.setBrowserName("webkit"); | ||
| config.contextOptions().setViewportSize(1280, 960); | ||
| return config; | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| void recordTraceInWebKit(Page page) { | ||
| page.navigate("https://playwright.dev"); | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yury-s which context options do we want the user to be able to set directly vs via providing ContextOptions? Do we want to have parity with the nodejs version.
Also in the node version you can specify certain options directly and also inside of certain objects like
launchOptionsandcontextOptions. Do we want to do this also? If so, which one takes precedent if both are provided and conflict with each other?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no consensus at the moment. It's nice to have a shortcut for the most common ones like
headless,viewportandbaseURLbut we are not sure if we need them for all properties.In Node.js all properties can be set directly but some of them have shortcuts too. We want some shortcuts, in Node.js it is programmed so that short form overrides long one. We can do the same in java, I don't think there was a strong reason for that order over the other, we just want the order to be deterministic.