Playwright API for Notebooks #14098

dhuebner · 2024-08-30T11:48:18Z

What it does

Adds Playwright page objects to interact with a notebook Editor and its cells.
Following actions are supported:

Open a Notebook Editor
Select a Kernel and read selected kernel name.
Adding a new Code/Markdown Cell
Read Cell mode (e.g. python, markdown etc.)
Getting cell editor text and adding text to the cell editor
Execute a cell
Read cell execution count
Read cell output

All of the above listed functionalities are tested in theia-notebook-editor.test.ts playwright test.

Additional note:

Playwright job now stores (for 2 days) test failure screenshots on failure. Results are in the zip called playwright-test-results and can be found in the action output.

How to test

Run theia-notebook-editor.test.ts mentioned above.

Follow-ups

Planned is to add other handy features like e.g. interaction with cell's toolbar or asserting error output.

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

dhuebner · 2024-08-30T14:21:51Z

The failing CI tests might indicates that a kernel installation is missing. I can check the Python setup for the corresponding GH action. Or maybe some expert could help faster?

examples/browser/package.json

dhuebner · 2024-09-04T11:59:01Z

Rebased on master and squashed commits.

msujew

This looks pretty good to me. However, I would like to get a review from someone more familiar with Playwright before merging this @JonasHelming do you think someone from ES could spare some time to review this?

planger

Thank you very much! This looks great. 👍
I just added a few inline uncritical comments.

planger · 2024-09-17T14:43:54Z

.github/workflows/playwright.yml

@@ -51,3 +58,11 @@ jobs:
      - name: Test (playwright)
        shell: bash
        run: yarn --cwd examples/playwright ui-tests-ci
+
+      - name: Archive test results
+        uses: actions/upload-artifact@v4


I think we try to pin actions to concrete git commit hashes for security reason. I think v4 resolves to 50769540e7f4bd5e21e526ee35c689e35e0d6874 at the moment.

I will change that, security first!

planger · 2024-09-17T14:54:52Z

examples/playwright/configs/playwright.ci.config.ts

@@ -21,6 +21,7 @@ const ciConfig: PlaywrightTestConfig = {
    ...baseConfig,
    workers: 1,
    retries: 2,
+    timeout: 30 * 1000,


Isn't that the default timeout? I'm not against adding it anyway, but I'm wondering why it was added.

I think we can remove it again.

Now, I know why it is explicitly listed. CI config extends the baseConfig where the time out is 60sec which is way too much IMHO.

planger · 2024-09-17T15:00:39Z

examples/playwright/src/tests/theia-notebook-editor.test.ts

+
+    test('kernels are installed', async () => {
+        editor = await app.openEditor('sample.ipynb', TheiaNotebookEditor);
+        await editor.tabLocator().click(); // force activate


I think it might be better to move that into the TheiaNotebookEditor page object, if this is really necessary. Otherwise every test would have to be aware that this needs to be called before subsequent interactions.

It was an attempt to fix flakiness on first run. I didn't found out why, but right after the start and opening an editor the focus jumps to the browser address field. Now I think it was because the "Workspace trust" message dialog was popping-up. It should no longer be necessary as the check is disabled in the example project package.json. I will remove this lines, let's see if the tests are passing in CI.

examples/playwright/src/theia-notebook-cell.ts

planger · 2024-09-17T15:05:37Z

examples/playwright/src/theia-notebook-cell.ts

+        const lines: string[] = [];
+        const linesCount = await this.monacoEditor.numberOfLines();
+        if (linesCount === undefined) {
+            return undefined;
+        }
+        for (let line = 1; line <= linesCount; line++) {
+            const lineText = await this.monacoEditor.textContentOfLineByLineNumber(line);
+            if (lineText === undefined) {
+                break;
+            }
+            lines.push(lineText);
+        }


Would it make sense to move this into the TheiaMonacoEditor? I guess this functionality could be useful in general for the text editor and would then be easier to test and maintain, if it is at a single place.

planger · 2024-09-17T15:06:20Z

examples/playwright/src/theia-notebook-cell.ts

+        const line = await this.editor.lineByLineNumber(lineNumber);
+        await line?.click();
+        await this.page.keyboard.type(text);


As above, I think it could make sense to move this behavior into the TheiaMonacoEditor.

planger · 2024-09-17T15:08:15Z

examples/playwright/src/theia-notebook-editor.ts

+     * @param kernelName  The name of the kernel to select.
+     */
+    async selectKernel(kernelName: string): Promise<void> {
+        // TODO should we use NotebookCommands.SELECT_KERNEL_COMMAND.id? Need a dependency...


Good question, I think I tend to avoid this dependency but rather introduce a namespace with constants here in the Playwright page object library.

planger · 2024-09-17T15:08:39Z

examples/playwright/src/theia-notebook-editor.ts

+    }
+
+    async availableKernels(): Promise<string[]> {
+        // TODO should we use NotebookCommands.SELECT_KERNEL_COMMAND.id? Need a dependency...


As above, I'd have a slight preference to introduce a namespace with constants in this page object file.

planger · 2024-09-17T15:11:18Z

examples/playwright/src/theia-quick-command-palette.ts

@@ -78,4 +78,14 @@ export class TheiaQuickCommandPalette extends TheiaPageObject {
        }
        return command.$('.monaco-list-row.focused .monaco-highlighted-label');
    }
+
+    async visibleItems(): Promise<ElementHandle<SVGElement | HTMLElement>[]> {


I think, we should add a dedicated test for that new method in examples/playwright/src/tests/theia-quick-command.test.ts.

Test is added

planger · 2024-09-17T15:15:35Z

packages/notebook/src/browser/view/notebook-main-toolbar.tsx

@@ -152,17 +152,17 @@ export class NotebookMainToolbar extends React.Component<NotebookMainToolbarProp

    override render(): React.ReactNode {
        const menuItems = this.getMenuItems();
-        return <div className='theia-notebook-main-toolbar'>
+        return <div className='theia-notebook-main-toolbar' id='notebook-main-toolbar'>


Is it save to use a static id here? Couldn't there be more than one toolbars in the DOM, eventually leading to non-unique IDs in the DOM? If yes, it might be better to use data-<xyz> attributes and locate them in the context of a parent?

I think it is okay to use a static id for the main toolbar. Should be only one, right? In case there are other sibling toolbars with the same id it can be handled as a list. For react one will need to adjust the key property.

There is one toolbar per editor, meaning that it's not unique. I.e. if you have two notebook editors side by side, it will no longer be unique.

They will be unique if using locator API instead of selector, what I do in notebook editor page object. Locator gives you the Notebook editor element under test, hence all subsequent queries are applied on the editor element and not the whole page.

dhuebner · 2024-09-19T12:38:53Z

I have no idea why it started to fail now.

dhuebner · 2024-09-19T14:57:18Z

Tests are running now. The cause was that a wrong Python 3.10 version was picked by default, where it should be Python 3.11. Fixed it now by requesting 3.11.* explicitly.

@planger
Thanks for your detailed feedback!
I've applied all of your suggestions except the one with the id for the main toolbar. I think it is not necessary and having a dynamic id will make the querying by ID inside the test more complex. Could you please review the current state?

dhuebner · 2024-09-19T15:29:37Z

Btw. the playwright reports are now created also on successful job run and are stored in job's Artifacts for two days. See for example: https://github.com/eclipse-theia/theia/actions/runs/10943260523?pr=14098

planger

Thanks a lot for the revision. Looks great to me!

dhuebner · 2024-09-20T07:23:08Z

@JonasHelming @msujew
Then this PR can be merged. I'm looking forward to see it in the upcoming .next release.

msujew · 2024-10-07T07:24:03Z

@dhuebner @planger FYI I would wait to merge this after #14234 has been merged. Just so we can fix unanticipated breakage in the playwright tests in this PR already.

msujew · 2024-10-08T14:18:30Z

@dhuebner Do you mind rebasing? #14234 has been merged.

Interact with monaco editor inside a cell Cell execute and get output functionality Fixed linting errors Cell Execution count access and test Install IPython kernel for the playwright job Download required plug-ins Fixed failing cell execution count test Move notebook related plugins to the root package.json Some clean-ups

Co-authored-by: Philip Langer <[email protected]>

dhuebner · 2024-10-09T13:35:26Z

@msujew
PR is ready to merge now. I've added an additional test to cover the new "split cell" action.

msujew · 2024-10-09T13:38:37Z

@dhuebner Great, thank you!

dhuebner force-pushed the dhuebner/notebook-ui-tests branch from 4360985 to cf61513 Compare August 30, 2024 12:14

dhuebner force-pushed the dhuebner/notebook-ui-tests branch 3 times, most recently from b26e767 to 3bf21ae Compare September 3, 2024 14:50

msujew reviewed Sep 3, 2024

View reviewed changes

examples/browser/package.json Outdated Show resolved Hide resolved

dhuebner force-pushed the dhuebner/notebook-ui-tests branch 2 times, most recently from 5bd36ce to a55c115 Compare September 4, 2024 11:57

dhuebner requested a review from msujew September 4, 2024 11:59

msujew reviewed Sep 17, 2024

View reviewed changes

JonasHelming requested a review from planger September 17, 2024 11:13

planger reviewed Sep 17, 2024

View reviewed changes

dhuebner force-pushed the dhuebner/notebook-ui-tests branch 2 times, most recently from 075c257 to 44db665 Compare September 19, 2024 12:09

dhuebner requested a review from planger September 19, 2024 14:57

planger approved these changes Sep 19, 2024

View reviewed changes

dhuebner force-pushed the dhuebner/notebook-ui-tests branch from e861cb3 to 0d24d30 Compare October 9, 2024 08:24

dhuebner and others added 6 commits October 9, 2024 13:45

Apply re-view suggestions

1f97987

Update examples/playwright/src/theia-notebook-cell.ts

5d5c969

Co-authored-by: Philip Langer <[email protected]>

Print available kernels to console. Added HTML report.

026fa43

Prefer Python 3.11 when running in CI

ae8fb60

Adjusted test API according to the new output webview structure

d266322

dhuebner force-pushed the dhuebner/notebook-ui-tests branch from 0d24d30 to d266322 Compare October 9, 2024 11:45

Added additional splitCell test

e422abb

msujew merged commit 6b7ceb7 into eclipse-theia:master Oct 9, 2024
11 checks passed

dhuebner deleted the dhuebner/notebook-ui-tests branch October 9, 2024 13:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Playwright API for Notebooks #14098

Playwright API for Notebooks #14098

dhuebner commented Aug 30, 2024 •

edited

Loading

dhuebner commented Aug 30, 2024

dhuebner commented Sep 4, 2024

msujew left a comment

planger left a comment

planger Sep 17, 2024

dhuebner Sep 19, 2024

planger Sep 17, 2024

dhuebner Sep 19, 2024

dhuebner Sep 19, 2024

planger Sep 17, 2024

dhuebner Sep 19, 2024

planger Sep 17, 2024

planger Sep 17, 2024

planger Sep 17, 2024

planger Sep 17, 2024

planger Sep 17, 2024

dhuebner Sep 19, 2024

planger Sep 17, 2024

dhuebner Sep 19, 2024

msujew Sep 19, 2024

dhuebner Sep 19, 2024

dhuebner commented Sep 19, 2024

dhuebner commented Sep 19, 2024

dhuebner commented Sep 19, 2024

planger left a comment

dhuebner commented Sep 20, 2024

msujew commented Oct 7, 2024

msujew commented Oct 8, 2024

dhuebner commented Oct 9, 2024

msujew commented Oct 9, 2024

Playwright API for Notebooks #14098

Playwright API for Notebooks #14098

Conversation

dhuebner commented Aug 30, 2024 • edited Loading

What it does

How to test

Follow-ups

Review checklist

Reminder for reviewers

dhuebner commented Aug 30, 2024

dhuebner commented Sep 4, 2024

msujew left a comment

Choose a reason for hiding this comment

planger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhuebner commented Sep 19, 2024

dhuebner commented Sep 19, 2024

dhuebner commented Sep 19, 2024

planger left a comment

Choose a reason for hiding this comment

dhuebner commented Sep 20, 2024

msujew commented Oct 7, 2024

msujew commented Oct 8, 2024

dhuebner commented Oct 9, 2024

msujew commented Oct 9, 2024

dhuebner commented Aug 30, 2024 •

edited

Loading