Improve CI speed #474

OskarDamkjaer · 2025-04-16T06:17:27Z

Based on #461

Splits e2e jobs to run in parallel
Adds cache on commit to avoid rebuilding
Cancel old jobs on the same pull request
Fixes parameter can't be set on system flaky test by improving the UX. We not instantly give the error (same as when disconnected), instead of letting the user fill in all the values and then giving an error ✨

On flakyness:

Ideally each test should be as isolated as possible. This is so they can run / re-run in parallel, etc. This means they need to ensure their own connection etc in a beforeEach. I'm not sure if this applies to the vscode tests though as it's not got multiple browser windows like playwright tests do 🤔

changeset-bot · 2025-04-16T06:17:31Z

⚠️ No Changeset found

Latest commit: de0cad3

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

OskarDamkjaer · 2025-04-16T13:39:07Z

.github/workflows/publish-vscode-extension.yaml

+      # todo verify caching works well here too
+      - name: Setup and build project
+        uses: ./.github/actions/setup-and-build


not sure how to best check for this

We could just make a card and check the next time we do a VSCode release?

OskarDamkjaer · 2025-04-16T13:40:03Z

packages/react-codemirror/playwright-ct.config.ts

-      use: { ...devices['Desktop Firefox'] },
-    },
-  ],
+  projects: [{ name: 'chromium', use: { ...devices['Desktop Chrome'] } }],


i think it makes sense to create a test suite dedicated for webkit + safari at a later stage and avoid the flakyness from firefox now. also happy to add firefox back if you prefer

I think we have firefox here because at some point we broke something in the semantic analysis that was firefox specific. To avoid regressions I think it'd be nice to still keep it in the CI?

We didn't fix the semantic analysis in firefox, it's still not working. I'll add it back and we can add skips if any test is very flaky in firefox only 👍

We didn't fix the semantic analysis in firefox, it's still not working.

Not completely you are right. But it does work in my Firefox, when it wasn't before. We definitely fixed something that wasn't working at some point that had to do with us setting the schema eagerly.

ncordon · 2025-04-16T14:25:12Z

packages/react-codemirror/package.json

@@ -28,7 +28,7 @@
    "test": "vitest run",
    "test:e2e": "playwright test -c playwright-ct.config.ts",
    "test:e2e-ui": "playwright test -c playwright-ct.config.ts --ui",
-    "benchmark": "BENCHMARKING=true npx playwright test -c playwright-ct.config.ts -g \"benchmarking & performance test session\""


Don't we need pnpx here?

pnpx will always download the latest from the internet and not use the locally installed version

For calling dependencies, we don't need a prefix (see how we call eslint, esbuild, playwright in the above command etc)

ncordon · 2025-04-16T14:27:33Z

packages/vscode-extension/package.json

-    "test:apiAndUnit": "pnpm build:dev && rm -rf .vscode-test/user-data && node ./dist/tests/runApiAndUnitTests.js",
-    "test:webviews": "pnpm build:dev && wdio run ./dist/tests/runWebviewTests.js"


Shouldn't this use a cached build? The reason why this builds the extension is to avoid to have to do that manually when developing locally.

The cache isn't as aggressive as turborepo, so this will rebuild. It's also rebuilding a dev build instead of a production build so we're not testing the same behavior that we'd send out to production.

For convenience locally it'd make more sense to have a new command, maybe test:webviews:watch or test:webviews:dev which rebuilds a development build and then runs the tests in some easy to debug mode

ncordon · 2025-04-16T14:30:06Z

packages/vscode-extension/src/commandHandlers/params.ts

+      'Parameters cannot be added when on the system database. Please connect to a user database.',
+    );


There's a message in the evaluateParameter which is slightly different:

'Parameters cannot be evaluated against a system database. Please connect to a user database.',

Probably it'd make sense to extract that to the constants.ts and have the same message everywhere?

The reason I made them slightly different was because the evaluating is at a later stage, differentiating between trying to add something and trying to validate the type of something while adding. Not sure if anyone really cares about the difference, so we could make both say "cannot be added"

ncordon · 2025-04-16T14:30:30Z

packages/vscode-extension/src/commandHandlers/params.ts

+    void window.showErrorMessage(
+      'Parameters cannot be edited when on the system database. Please connect to a user database.',
+    );


Also extract this?

makes sense 👍

ncordon · 2025-04-16T14:34:19Z

packages/vscode-extension/tests/specs/webviews/params.spec.ts

+    void browser.executeWorkbench((vscode) => {
      // eslint-disable-next-line @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-member-access
-      await vscode.commands.executeCommand('neo4j.internal.forceDisconnect');
+      void vscode.commands.executeCommand('neo4j.internal.forceDisconnect');
    });


At some point I was using the voids as well, but that meant we had a notification that unless closed, the method wasn't finishing?

I think we should just swap all of the:

await window.showErrorMessage

for:

void window.showErrorMessage

instead of having void here?

we had a notification that unless closed, the method wasn't finishing?
what does this mean? This method still blocks like before due to the new await waitUntilNotification below

I'm not implying we should change anything here but that we should replace the await window.showErrorMessage with void window.showErrorMessage everywhere in the codebase? I don't see any reason why that should block the execution in general of some method when things went wrong

i mean the test will fail if the show error message goes wrong no? The very next assertion is waiting for it to fire?

ncordon · 2025-04-16T14:35:36Z

packages/vscode-extension/tests/webviewUtils.ts

+  const notifications = await wb.getNotifications();
+  for (const notification of notifications) {
+    await notification.dismiss();
+  }
+
+  const remainingNotifications = await wb.getNotifications();


Interesting. Why do we need to get notifications twice?

In my testing overflowing ones didn't close get dismissed, so I needed to call it more than once. I think it makes sense to take some time and look at the docs to see if there's some "clear notifications" thing we could run instead

ncordon · 2025-04-16T14:47:59Z

.github/workflows/ci.yaml

+concurrency:
+  group: ${{ github.workflow }}-${{ github.ref }}
+  cancel-in-progress: ${{ github.event_name == 'pull_request' }}
+


I like this because it prevents us from kicking jobs for different commits at the same time, but there's one thing about this I don't quite like: if two different jobs fail for the latest commit, you either have to retrigger them all or re-trigger one, wait for that to pass and re-trigger the second one.

I've looked it up and there's no way to tweak the concurrency per individual job, only per workflow. Any thoughts?

Is there not a "retry all failed steps" in the UI that works for this?

ncordon

So excited about this!!!! 🚀

ncordon · 2025-04-17T09:30:10Z

I'm going to merge this, given the CI is super flaky, we can address anything extra in other PRs

Copilot

Pull Request Overview

This PR aims to improve CI speed by introducing parallel execution, caching to prevent redundant rebuilds, and UX improvements in parameter error handling. Key changes include:

Splitting e2e jobs to run in parallel with updated workflow configurations.
Introducing caching on commit and replacing legacy build steps with a new "setup-and-build" action.
Enhancing error messaging for parameter handling, especially on system databases.

Reviewed Changes

Copilot reviewed 14 out of 16 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
packages/vscode-extension/tests/webviewUtils.ts	Improved notification handling by throwing errors instead of returning false and added a recursive dismiss function.
packages/vscode-extension/tests/specs/webviews/params.spec.ts	Updated test flows by switching to non-awaited executeWorkbench calls and refining notification waits.
packages/vscode-extension/tests/specs/api/autoCompletion.spec.ts	Reordered tests to ensure parameter completions work correctly and fixed variable naming inconsistencies.
packages/vscode-extension/src/commandHandlers/params.ts	Added checks to prevent parameter modifications when connected to the system database.
packages/vscode-extension/src/commandHandlers/connection.ts	Streamlined disconnect flow by replacing message display calls.
packages/react-codemirror/playwright-ct.config.ts	Adjusted configurations for timeout and parallel execution improvements.
.github/workflows/*.yaml and .github/actions/setup-and-build/action.yaml	Updated CI workflows to use the new setup-and-build process and improve caching and concurrency.

Files not reviewed (2)

packages/react-codemirror/package.json: Language not supported
packages/vscode-extension/package.json: Language not supported

Comments suppressed due to low confidence (1)

packages/vscode-extension/tests/webviewUtils.ts:230

The recursive call in ensureNotificationsAreDismissed may lead to an infinite loop if notifications persist; consider adding a delay or a maximum retry limit to prevent potential stack overflow.

if (remainingNotifications.length > 0) { return ensureNotificationsAreDismissed(browser); }

Copilot · 2025-04-22T19:05:39Z

packages/vscode-extension/tests/specs/webviews/params.spec.ts

+    void browser.executeWorkbench((vscode) => {
      // eslint-disable-next-line @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-member-access
-      await vscode.commands.executeCommand('neo4j.internal.forceDisconnect');
+      void vscode.commands.executeCommand('neo4j.internal.forceDisconnect');


Using 'void' instead of 'await' here may swallow errors from executeWorkbench; consider awaiting the call to handle possible failures.

Copilot · 2025-04-22T19:05:40Z

packages/vscode-extension/tests/specs/webviews/params.spec.ts

  }

  async function forceConnect(i: number) {
-    await browser.executeWorkbench(async (vscode, i) => {
+    void browser.executeWorkbench((vscode, i) => {


Using 'void' instead of 'await' here may hide errors from the asynchronous operation; ensure proper error handling by awaiting the executeWorkbench call.

Suggested change

void browser.executeWorkbench((vscode, i) => {

await browser.executeWorkbench((vscode, i) => {

Copilot · 2025-04-22T19:05:40Z

packages/vscode-extension/tests/specs/api/autoCompletion.spec.ts

+    );
+    const textDocument = await newUntitledFileWithContent(`RETURN `);
+    const position = new vscode.Position(0, 7);
+    const expecations: vscode.CompletionItem[] = [


[nitpick] The variable name 'expecations' appears to be a typo; consider renaming it to 'expectations'.

Suggested change

const expecations: vscode.CompletionItem[] = [

const expectations: vscode.CompletionItem[] = [

OskarDamkjaer requested a review from ncordon April 16, 2025 12:26

OskarDamkjaer marked this pull request as ready for review April 16, 2025 12:26

squash commit

de0cad3

OskarDamkjaer force-pushed the faster_ci branch from e5ac9c2 to de0cad3 Compare April 16, 2025 13:31

self review

de1e1cb

OskarDamkjaer commented Apr 16, 2025

View reviewed changes

ncordon reviewed Apr 16, 2025

View reviewed changes

ncordon mentioned this pull request Apr 17, 2025

Fix e2e tests flakiness #473

Closed

ncordon approved these changes Apr 17, 2025

View reviewed changes

ncordon merged commit 61ec95d into main Apr 17, 2025
6 checks passed

ncordon deleted the faster_ci branch April 17, 2025 09:30

ncordon requested a review from Copilot April 22, 2025 19:04

Copilot AI reviewed Apr 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve CI speed #474

Improve CI speed #474

OskarDamkjaer commented Apr 16, 2025 •

edited

Loading

changeset-bot bot commented Apr 16, 2025 •

edited

Loading

OskarDamkjaer Apr 16, 2025

ncordon Apr 16, 2025

OskarDamkjaer Apr 16, 2025

ncordon Apr 16, 2025

OskarDamkjaer Apr 16, 2025 •

edited

Loading

ncordon Apr 16, 2025

ncordon Apr 16, 2025

OskarDamkjaer Apr 16, 2025

OskarDamkjaer Apr 16, 2025

ncordon Apr 16, 2025

OskarDamkjaer Apr 16, 2025

ncordon Apr 16, 2025

OskarDamkjaer Apr 16, 2025

ncordon Apr 16, 2025

OskarDamkjaer Apr 16, 2025

ncordon Apr 16, 2025 •

edited

Loading

OskarDamkjaer Apr 16, 2025

ncordon Apr 16, 2025

OskarDamkjaer Apr 22, 2025

ncordon Apr 16, 2025

OskarDamkjaer Apr 16, 2025

ncordon Apr 16, 2025

OskarDamkjaer Apr 16, 2025

ncordon left a comment

ncordon commented Apr 17, 2025

Copilot AI left a comment

Copilot AI Apr 22, 2025

Copilot AI Apr 22, 2025

Copilot AI Apr 22, 2025

		"test:apiAndUnit": "pnpm build:dev && rm -rf .vscode-test/user-data && node ./dist/tests/runApiAndUnitTests.js",
		"test:webviews": "pnpm build:dev && wdio run ./dist/tests/runWebviewTests.js"

		'Parameters cannot be added when on the system database. Please connect to a user database.',
		);

	void browser.executeWorkbench((vscode, i) => {
	await browser.executeWorkbench((vscode, i) => {

	const expecations: vscode.CompletionItem[] = [
	const expectations: vscode.CompletionItem[] = [

Improve CI speed #474

Improve CI speed #474

Conversation

OskarDamkjaer commented Apr 16, 2025 • edited Loading

changeset-bot bot commented Apr 16, 2025 • edited Loading

⚠️ No Changeset found

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

OskarDamkjaer Apr 16, 2025 • edited Loading

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

ncordon Apr 16, 2025 • edited Loading

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

ncordon left a comment

Choose a reason for hiding this comment

ncordon commented Apr 17, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Copilot AI Apr 22, 2025

Choose a reason for hiding this comment

Copilot AI Apr 22, 2025

Choose a reason for hiding this comment

Copilot AI Apr 22, 2025

Choose a reason for hiding this comment

OskarDamkjaer commented Apr 16, 2025 •

edited

Loading

changeset-bot bot commented Apr 16, 2025 •

edited

Loading

OskarDamkjaer Apr 16, 2025 •

edited

Loading

ncordon Apr 16, 2025 •

edited

Loading