Start converting tests to Playwright #307

darabos · 2022-11-21T15:41:48Z

First step for #238. Includes a conversion of web/test/tests/splash-page-test.js.

I haven't updated the docs yet. You can see the CI setup in the Earthfile. For local development you install Playwright the same way. Then you run LynxKite with run.sh and run the tests with yarn playwright test. There's a nice debug utility you can start with yarn playwright debug. If you hit an issue that you can't reproduce with debug, you can run the tests with yarn playwright test --trace on. This will save a trace that you can replay to see what happened.

I want to set up the trace on GitHub Actions too. The trace will have to be saved as an Earthly local artifact, then saved as a GitHub Actions artifact. This will be fantastic to track down any flaky tests. Or maybe we just won't have flaky tests at all!

darabos · 2022-11-21T16:34:10Z

web/tests/lynxkite.ts

@@ -0,0 +1,1596 @@
+import { expect, Locator, Page } from '@playwright/test';


This file is a garbage dump at the moment. It started as a copy of web/test/test-lib.js, then I converted the parts that I used to Playwright.

I think in many places this library is overdoing it. Is expectDirectoryNotListed(name) any clearer than expect(page.splash.directory(name)).not.toBeVisible()?

My approach would be to convert the tests fairly faithfully in the first round. Then we can have another pass to clean up a bit.

darabos · 2022-11-21T16:48:16Z

web/tests/splash.spec.ts

+  return page;
+}
+
+test.describe.configure({ mode: 'serial' });


The Protractor tests have an intricate test framework implemented in web/test/declarative.js. Instead of a simple independent/serial execution this allows a tree of tests, where some tests preserve the state and some tests change it.

This is made possible by the discardAllReallyIMeanIt endpoint, which resets LynxKite completely and provides a safe starting state. I'm not fond of this, because I often have some nice workspaces for testing stuff I'm working on, and then the tests blow it away.

My hope is that we can ditch all this. We can get to a known state by (re)creating a directory for the tests. We can write each test file to be runnable on its own. They can still share code: another test could import newSplash() from this file and just call it. We don't need to build a framework for this.

If we put the test stuff for each test file into a separate LynxKite directory, perhaps we could one day run those in parallel. But it's fine not to worry about this for now.

darabos · 2022-11-24T18:16:21Z

On my laptop when I run all the tests, something goes wrong when "SQL runs nice on belongs to reached from project and segmentation" tries to add the "Create vertices" box. It clicks on it but the popup doesn't come up. I think this may be an actual LynxKite bug.

A difference between Protractor and Playwright is that Protractor waited for network requests to finish before taking the next action. Playwright doesn't care. So it may be clicking the box before the getOrSetWorkspace call has finished. Which is fine! A user wouldn't wait either! And I have actually experienced clicking boxes and not getting popups. Fixing things for Playwright also fixes things for users.

darabos · 2022-11-24T18:25:03Z

It passed on GitHub Actions. I guess let's merge it! (After review.) Then we can start converting more tests and fixing things as we go.

darabos · 2022-11-24T18:26:29Z

web/tests/sql.spec.ts

+  await tableBrowser.expectNode([3, 1], 'age (Double)', '`age`');
+});
+
+/*


These are commented-out in the original. But let's fix what we can and delete what we no longer need during this migration.

These are commented-out in the original. But let's fix what we can and delete what we no longer need during this migration.

I've reviewed them. Nothing worth fixing I think. I've deleted them all.

darabos

I've managed to track down some reliability issues. Looks rock solid for me now!

darabos · 2022-11-25T10:08:02Z

web/tests/sql.spec.ts

+  await tableBrowser.expectNode([3, 1], 'age (Double)', '`age`');
+});
+
+/*


These are commented-out in the original. But let's fix what we can and delete what we no longer need during this migration.

I've reviewed them. Nothing worth fixing I think. I've deleted them all.

darabos · 2022-11-25T10:15:58Z

web/tests/lynxkite.ts

+        { logicalX: boxData.x, logicalY: boxData.y },
+        { boxId: boxData.id });
+    }, boxData);
+    // Wait for the backend to save this box.


Alternatively we could make it so that it's okay to click a box before it's saved. I've filed #308.

lacca0 · 2022-11-27T18:12:07Z

.github/workflows/earthly-test.yml

@@ -33,13 +34,14 @@ jobs:
        fi
        git checkout -b "$branch" || true
    - name: Download latest earthly
-      run: "sudo /bin/sh -c 'wget https://github.com/earthly/earthly/releases/download/v0.6.26/earthly-linux-amd64 -O /usr/local/bin/earthly && chmod +x /usr/local/bin/earthly'"
+      run: "sudo /bin/sh -c 'wget https://github.com/earthly/earthly/releases/download/v0.6.30/earthly-linux-amd64 -O /usr/local/bin/earthly && chmod +x /usr/local/bin/earthly'"


Why can't we use /earthly/releases/latest/...?

Why can't we use /earthly/releases/latest/...?

With a fixed version the behavior is more predictable. With /latest there would always be a chance that something broke because of a new Earthly release.

(I'm in favor of a lockfile for the Conda dependencies too, for the same reason.)

But maybe we can switch to /latest when Earthly becomes stable enough, e.g. 1.0.

lacca0 · 2022-11-27T19:38:33Z

Earthfile

@@ -1,4 +1,4 @@
-VERSION --use-copy-include-patterns 0.6
+VERSION --use-copy-include-patterns --try 0.6


Cannot find this option at docs.earthly.dev. What does it do?

Cannot find this option at docs.earthly.dev. What does it do?

Yeah, it's pretty new. See earthly/earthly#587 (comment).

Unfortunately it only works for files under 64 kB at the moment. (earthly/earthly#2452) We will only start getting test reports when a new Earthly is released with the fix.

lacca0 · 2022-11-27T19:38:35Z

Earthfile

@@ -25,24 +25,24 @@ sbt-deps:

 npm-deps:
  COPY web/package.json web/yarn.lock web/
-  RUN cd web; yarn --frozen-lockfile
+  RUN cd web && yarn --frozen-lockfile


Good idea!

For cd it doesn't make much difference. But I used ; instead of && at some other step and ended up ignoring a failure as a result. I took revenge on all semicolons.

lacca0 · 2022-11-27T19:38:45Z

tools/with_lk.sh

-KITE_DEPLOYMENT_CONFIG_DIR=$(realpath "${KITE_DEPLOYMENT_CONFIG_DIR}")
-export HTTP_PORT=$[ 9100 + RANDOM % 100 ]
-export HTTPS_PORT=$[ 9200 + RANDOM % 100 ]
+export KITE_HTTP_PORT=$[ 9100 + RANDOM % 100 ]


Why did we need to make these ports random at the first place?

Why did we need to make these ports random at the first place?

I think it's to make it more convenient for local testing. You usually run a LynxKite on port 2200 that you're working with. You can use that for testing. This script is for starting a different, brand new LynxKite just for the test.

For testing in Earthly it doesn't matter. A fixed port would be fine.

lacca0

Thanks!

darabos · 2022-11-30T10:40:49Z

I was considering whether to merge this before the release. I think it's fine. The enqueue fix is a small change. Playwright likes it. It looks okay in manual testing. I've run the Protractor tests and they pass too.

darabos added 8 commits November 14, 2022 13:47

Add Playwright and initialize it.

9a5351f

Start converting a test to Playwright.

d8e1c78

Merge branch 'darabos-html-tests-docs' into darabos-playwright

98bcbe7

First test is passing!

af130f0

Splash tests all working! (+some formatting changes)

84185bf

Respect LYNXKITE_ADDRESS in Playwright.

409e11e

Playwright in Earthly.

cee73fe

Add to LD_LIBRARY_PATH instead of replacing it.

729c62e

darabos requested a review from lacca0 November 21, 2022 15:41

darabos commented Nov 21, 2022

View reviewed changes

darabos added 4 commits November 21, 2022 18:15

Names for GitHub Actions jobs.

b153724

No need to set LYNXKITE_ADDRESS for tests now.

ac51fd2

Get started with converting SQL tests.

efd59ae

Simpler GitHub Actions workflow name.

1f915ee

Base automatically changed from darabos-html-tests-docs to main November 24, 2022 13:41

darabos added 3 commits November 24, 2022 16:56

Some SQL tests are passing.

737ed5a

More SQL tests passing.

540365e

SQL tests mostly working.

04beb76

darabos commented Nov 24, 2022

View reviewed changes

darabos added 2 commits November 25, 2022 10:50

Remove old dead tests. Nothing to salvage I think.

7e48ac3

Make directory and box creation more reliable.

d1d78df

darabos force-pushed the darabos-playwright branch from 852dd07 to d1d78df Compare November 25, 2022 10:13

darabos commented Nov 25, 2022

View reviewed changes

darabos mentioned this pull request Nov 25, 2022

Clicking a box doesn't open its popup until it's saved #308

Closed

Try to set up saving test reports on GitHub Actions.

a924900

darabos force-pushed the darabos-playwright branch 2 times, most recently from 00c6004 to a0d1d99 Compare November 25, 2022 17:02

Looks like Earthly doesn't support directories in FINALLY.

03b1571

darabos force-pushed the darabos-playwright branch from a0d1d99 to 03b1571 Compare November 25, 2022 17:04

lacca0 reviewed Nov 27, 2022

View reviewed changes

darabos added 2 commits November 29, 2022 10:54

Move shared test code to lynxkite.ts.

0ffa28c

Upgrade docker/login-action because of warnings.

89d37da

lacca0 approved these changes Nov 29, 2022

View reviewed changes

Make sure delayed actions are executed in order.

3b279b5

darabos merged commit 51aadc3 into main Nov 30, 2022

darabos deleted the darabos-playwright branch November 30, 2022 10:40

lacca0 mentioned this pull request Dec 13, 2022

Rewrite example graph test #316

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start converting tests to Playwright #307

Start converting tests to Playwright #307

darabos commented Nov 21, 2022

darabos Nov 21, 2022

darabos Nov 21, 2022 •

edited

darabos commented Nov 24, 2022

darabos commented Nov 24, 2022

darabos Nov 24, 2022

darabos Nov 25, 2022

darabos left a comment

darabos Nov 25, 2022

darabos Nov 25, 2022

lacca0 Nov 27, 2022

darabos Nov 28, 2022

lacca0 Nov 27, 2022

darabos Nov 28, 2022

lacca0 Nov 27, 2022

darabos Nov 28, 2022

lacca0 Nov 27, 2022

darabos Nov 28, 2022

lacca0 left a comment

darabos commented Nov 30, 2022

		@@ -0,0 +1,1596 @@
		import { expect, Locator, Page } from '@playwright/test';

		@@ -1,4 +1,4 @@
		VERSION --use-copy-include-patterns 0.6
		VERSION --use-copy-include-patterns --try 0.6

Start converting tests to Playwright #307

Start converting tests to Playwright #307

Conversation

darabos commented Nov 21, 2022

Choose a reason for hiding this comment

darabos Nov 21, 2022 • edited

Choose a reason for hiding this comment

darabos commented Nov 24, 2022

darabos commented Nov 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darabos 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

lacca0 left a comment

Choose a reason for hiding this comment

darabos commented Nov 30, 2022

darabos Nov 21, 2022 •

edited