-
Notifications
You must be signed in to change notification settings - Fork 92
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
e2e-test: add test cov for 'open changes' in editor action bar #5836
Conversation
E2E Tests 🚀 ? |
await test.step('verify "open in viewer" renders html', async () => { | ||
await app.code.driver.page.getByLabel('Open in Viewer').nth(1).click(); | ||
const viewerFrame = app.code.driver.page.locator('iframe.webview').contentFrame().locator('#active-frame').contentFrame(); | ||
const cellLocator = app.web |
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.
should some of this be in positronViewer? It seems like there is a bit of duplication
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.
Maybe? I'm on the fence. I usually don't put verifications in the POM, but certainly interacting with the buttons could be in there. I can do a followup PR to move some of this into the PositronViewer?
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.
Or would it be PositronEditor? Maybe both, I guess I'll find out...
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.
yeah, agreed about verifications in the POM. we definitely have some but not my favorite. was thinking more about locators that might be reusable
async function bindPlatformHotkey(page: Page, key: string) { | ||
await page.keyboard.press(process.platform === 'darwin' ? `Meta+${key}` : `Control+${key}`); | ||
} | ||
|
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.
seems like we may need a new place to start collecting things like this. i.e. general purpose helper functions. not necessary for this PR but maybe keep in mind that we might want a positronCode.ts or something
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.
(and that might be a good step towards getting unhooked from the MS dependencies)
Would be great if everything didn't depend on code.ts since code.ts is loaded with getElements, etc
argh
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.
Made some comments, but I think they are all follow up stuff!
LGTM!
Enhanced test coverage for the editor action bar, specifically verifying that ‘Open Changes’ functions as expected for
.qmd
files. Additionally, refined test readability for better clarity and maintainability.QA Notes
Tagged the test to run in this PR. If it passes, we are good. 🙌
@:editor-action-bar