-
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
Conversation
| } | ||
|
|
||
| public void setStorageStatePath(Path path) { | ||
| contextOption.setStorageStatePath(path); |
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 launchOptions and contextOptions. 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.
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.
There is no consensus at the moment. It's nice to have a shortcut for the most common ones like headless, viewport and baseURL but we are not sure if we need them for all properties.
Also in the node version you can specify certain options directly and also inside of certain objects like launchOptions and contextOptions. Do we want to do this also? If so, which one takes precedent if both are provided and conflict with each other?
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.
| return apiRequestOptions; | ||
| } | ||
|
|
||
| public <T> void setClientValue(String name, T value) { |
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.
is this for setting PlaywrightOptions?
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.
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.
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.
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.
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.
Let's omit these two methods for now. Even if we expose Config object it won't have a getter setter for myCustomProperty and this is essentially a custom hash map on the config.
No description provided.