-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[scout] An abstraction for extending pageObjects #243111
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
base: main
Are you sure you want to change the base?
Conversation
|
🤖 Jobs for this PR can be triggered through checkboxes. 🚧
ℹ️ To trigger the CI, please tick the checkbox below 👇
|
| return { | ||
| ...initedLazyPageObjects, | ||
| ...pageObjects, | ||
| } as PageObjectsExtended; |
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.
Would prefer a better solution than using as PageObjectsExtended but it was the best I could do.
Also, I bet a number of these things could be better named.
| * License v3.0 only", or the "Server Side Public License, v 1". | ||
| */ | ||
|
|
||
| import { test as base } from '../../../..'; |
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.
looks like this could be done for the lighthouseTest and spaceTest as well
|
|
||
| type PageObjectClass = new (page: ScoutPage) => AbstractPageObject; | ||
|
|
||
| export const createTest = function <PageObjectsExtensions = Record<string, AbstractPageObject>>( |
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.
Perhaps extendTest or extendPageObjects would make more sense
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
|
|
|
||
| await use(extendedPageObjects); | ||
| }, | ||
| export const test = createTest<{ |
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.
This is the result I desired, a simple and terse way to extend the set of page objects. The createTest function is dense and difficult to read but the code it replaces didn't have any more meaning as best I could tell.
|
|
||
| type PageObjectClass = new (page: ScoutPage) => AbstractPageObject; | ||
|
|
||
| export const createTest = function <PageObjectsExtensions = Record<string, AbstractPageObject>>( |
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.
createTest() is only extending the pageObjects fixture (test-scoped) but notice it still passes two type parameters to base.extend(): test-scoped fixtures and worker-scoped fixtures. It seems to me here we limit the scope of what you can do with createTest to just page object creation, which imho isn't ideal.
See some examples of Scout packages and plugins extending multiple fixtures (not just pageObjects) using base.extend():
- The Security Scout package extends both
pageObjectsandapiServiceshere. - The Scout Observability package is extending both fixtures as well here.
- The APM plugin extends both
pageObjectsandbrowserAuthhere.
What I appreciate about the current architecture is how flexible it is when it comes to extending any Scout core fixture (test-scoped and worker-scoped) to expose additional methods. While your method is convenient I'm afraid it will limit what plugins (but most especially Scout packages) can expose to other plugins. What do you think?
Summary
There's a bit of repeated boilerplate code necessary for adding pageObject functionality. This PR abstracts it so it can be replaced with a single line of code.
Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
release_note:breakinglabel should be applied in these situations.release_note:*label is applied per the guidelinesbackport:*labels.Identify risks
Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.
Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.