-
Notifications
You must be signed in to change notification settings - Fork 285
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
WSL E2E tests: add more new locators #5315
base: main
Are you sure you want to change the base?
Conversation
- Allow updating the config file to fail first few writes. - Must force the clicks. - Fix up the 'gamma' tests: There's no need to click on it, because it isn't enabled. Reload the preference page to verify the new value is in place. Signed-off-by: Eric Promislow <[email protected]>
Signed-off-by: Eric Promislow <[email protected]>
}); | ||
|
||
test('should allow enabling integration', async() => { | ||
// This is how we do a reload... |
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.
I don't think we're doing a reload here, just casting preferencesWindow
(which is a Page
) to a Page
subclass that knows how to handle the specifics of that window, and then grabbing the wslPage
off it.
expect(await alpha.isChecked()).toBeFalsy(); | ||
expect(await alpha.isEnabled()).toBeTruthy(); | ||
// Don't know why force-true is necessary, playwright times out without it. | ||
await alpha.click({ force: true }); |
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.
Note that this is forcing a click on the <input type="checkbox">
, but normally we'd be interacting with the owning <Checkbox>
Vue component instead… That's why we had WSLIntegrationsPage.getIntegration(): CheckboxLocator
.
await writeConfig({ alpha: true }); | ||
await alpha.assertEnabled(); | ||
await expect(alpha.checkbox).toBeChecked(); | ||
// Now 'relocate' alpha |
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 comment is a bit too punny and obscures what it's actually trying to explain… It's probably better to rewrite it to be clearer.
Or maybe just drop the local variable and always use wslPage.alpha
instead?
|
||
await expect(newGamma.error).toHaveText('some other error'); | ||
await newGamma.assertDisabled(); | ||
// The `isDisabled` locator simply doesn't work -- possibly because the actual DOM is |
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.
See above re: WSLIntegrationsPage.getIntegration(): CheckboxLocator
.
DO NOT MERGE -- FOR DISCUSSION
... except I have a final test,
'should see new invalid reason'
, and it doesn't seem to reflect the new config. Maybe the mocker hasn't loaded the new config.Also the last test
await expect(parent.filter({ hasText: newErrorMessage })).toHaveCount(0);
passes, but I think thayt0
should be a1
.Also I can't get the
isDisabled()
locator to work, because according to the DOM layout in the debugger, playwright doesn't have a locator pointing at the thing that's actually disabled (it's the label element that wraps the checkbox).This PR needs discussion on what we need to do to get the last test to do useful things.