Addressing issues with failing flaky tests by johnsimons · Pull Request #2630 · Particular/ServicePulse

johnsimons · 2025-09-15T23:44:00Z

We found an issue with the cleaning-up routines that can cause certain instances to be cleaned up before others.
We have started a discussion in vitest to figure out if the behaviour is a bug or if we are just using it incorrectly.

In the meantime, we have addressed the issue by refactoring the code to do the cleanup in a more consistent way.

This is to fix the flaky test failures that cause: Error: ReferenceError: window is not defined ❯ stopTimer src/composables/autoRefresh.ts:12:7 ❯ startTimer src/composables/autoRefresh.ts:20:5 ❯ executeAndResetTimer src/composables/autoRefresh.ts:31:7 ❯ Timeout._onTimeout src/composables/autoRefresh.ts:22:7 ❯ listOnTimeout node:internal/timers:588:17 ❯ processTimers node:internal/timers:523:7

Added run to vitest so by default we not running in watch mode

Otherwise the cleanup happens in a non deterministic order

johnsimons · 2025-10-07T05:39:18Z

    "lint": "eslint .",
    "preview": "vite preview",
-    "test:component": "vitest ./src",
+    "test:component": "vitest run ./src",


By default vitest runs in watch mode, but in our CI we always want it to perform a single run without watch mode.

This was able to detect that it was running from CI and not hold the execution in an infinite watch. Making it explicit with run is good 👍

johnsimons · 2025-10-07T05:39:49Z

    "@testing-library/vue": "8.1.0",
    "@tsconfig/node18": "18.2.4",
    "@types/bootstrap": "5.2.10",
+    "@types/jsdom": "27.0.0",


We were missing this typescript type

johnsimons · 2025-10-07T05:41:09Z

-function deleteAllCookies() {
-  const cookies = document.cookie.split(";");
-
-  for (let i = 0; i < cookies.length; i++) {
-    const cookie = cookies[i];
-    const eqPos = cookie.indexOf("=");
-    const name = eqPos > -1 ? cookie.substr(0, eqPos) : cookie;
-    document.cookie = name + "=;expires=Thu, 01 Jan 1970 00:00:00 GMT";
-  }
-}
-
-afterEach(() => {
-  //Intentionally not calling mockServer.resetHandlers here to prevent ServicePulse in flight messages after app unmount to fail.
-  //mockServer.resetHandlers is being called instead in driver.ts
-
-  //Make JSDOM create a fresh document per each test run
-  jsdom.reconfigure({ url: "http://localhost:3000/" });
-  localStorage.clear();
-  sessionStorage.clear();
-  deleteAllCookies();


All this code was moved into the driver.ts

johnsimons · 2025-10-07T05:42:09Z

-  deleteAllCookies();
+afterAll(() => {
+  console.log("Shutting down mock server");
+  mockServer.close();


We are cleaning up the mock server as per the documentation

I am glad that this call to close now works. The await flushpromises now being part of the driver probably does the trick. Nice job 👍

johnsimons · 2025-10-07T05:42:25Z

  },
  test: {
-    pool: "forks", //https://github.com/vitest-dev/vitest/issues/2008#issuecomment-187106690
+    pool: "forks",


forks is now the default.

If forks is now the default, do we still need to explicitly configure it?

I left it just to be sure

This file is auto generated by msw

Added more diagnostics so we can see what is going on

This test is just validating what SC does. No need for it.

…ished

johnsimons · 2025-10-14T07:53:16Z

@cquirosj, if you don't mind, can you review this PR?
There is quite a bit to it, and there is also this PR that explains some of the changes.
The TL;DR is that we divorced the store from the auto refreshes and made the auto refresh based on the component lifecycle (mount/unmount) so that, for example, the refresh stops if all components are unmounted.
We also found an issue with the cleaning-up routines that can cause certain instances to be cleaned up before others.
We have started a discussion in vitest to figure out if the behaviour is a bug or if we are just using it incorrectly, and it seems it is by design, so we ended up moving things around to do with the tests cleanup to ensure things are cleaned up in the correct order.

cquirosj · 2025-10-14T22:03:45Z

    "lint": "eslint .",
    "preview": "vite preview",
-    "test:component": "vitest ./src",
+    "test:component": "vitest run ./src",


This was able to detect that it was running from CI and not hold the execution in an infinite watch. Making it explicit with run is good 👍

cquirosj · 2025-10-14T22:04:33Z

  },
  test: {
-    pool: "forks", //https://github.com/vitest-dev/vitest/issues/2008#issuecomment-187106690
+    pool: "forks",


If forks is now the default, do we still need to explicitly configure it?

cquirosj · 2025-10-14T22:06:38Z

-  deleteAllCookies();
+afterAll(() => {
+  console.log("Shutting down mock server");
+  mockServer.close();


I am glad that this call to close now works. The await flushpromises now being part of the driver probably does the trick. Nice job 👍

cquirosj · 2025-10-14T22:08:14Z

+    console.log("Starting test");
+
    //run the test
    await use(driver);

+    console.log("Test ended");
    //unmount the app after the test runs
    driver.disposeApp();
+
+    // We need to wait for any pending promises to resolve before resetting handlers and clearing storage
+    await flushPromises();
+
+    console.log("Cleanup after test");


Were these console.log statements left here intentionally to be part of the PR? If so, that is ok.

yeah, I left them intentionally to provide better diagnostics for the tests

johnsimons requested a review from ngallegos September 15, 2025 23:44

internalautomation Bot assigned johnsimons Sep 15, 2025

johnsimons force-pushed the john/flaky branch from b77a723 to 02dec82 Compare September 16, 2025 06:21

ngallegos mentioned this pull request Sep 18, 2025

Flaky test: test/specs/monitoring/filtering-endpoints.spec.ts #2631

Open

ngallegos force-pushed the john/flaky branch from 42de1fd to 02dec82 Compare September 23, 2025 22:39

johnsimons added 4 commits October 7, 2025 14:28

forks is now the default

a78df4c

adding jsdom types

f3923b4

Added run to vitest so by default we not running in watch mode

Moved afterEach into the driver

e00aa93

Otherwise the cleanup happens in a non deterministic order

johnsimons force-pushed the john/flaky branch from 02dec82 to e00aa93 Compare October 7, 2025 04:28

johnsimons changed the title ~~Replace window.setTimeout with just setTimeout~~ Addressing issues with failing flaky tests Oct 7, 2025

johnsimons requested a review from PhilBastian October 7, 2025 05:37

johnsimons commented Oct 7, 2025

View reviewed changes

Fix failing tests

c928e82

johnsimons force-pushed the john/flaky branch from 779c6e4 to c928e82 Compare October 7, 2025 05:43

johnsimons added 15 commits October 8, 2025 19:09

Extract auto refresh from stores

13f7cd3

Rework tests based on changes

01b25e0

Using the flush-promises from vitest api

bdf5b1f

Installed vite-plugin-vue-devtools for diagnostics during development

d02c45d

Need to flush promises in driver too

f41bf1a

Use latest windows

8562082

Increase timeout slightly to 15 seconds

d9a01a1

Remove unneeded props

af1b666

Clean up the watch handler

a20e127

Log a warning if a call comes that is not handled

4bb15f5

We need to pause and resume the auto-refresh

8bfa986

No need to lint mockServiceWorker.js

c3bc851

This file is auto generated by msw

Simplified start of initial refreshes

e51e1ed

Reworked the auto refresh

001989f

Added more diagnostics so we can see what is going on

Fix autoRefresh to properly handle store setting

3a0911b

johnsimons added 5 commits October 13, 2025 14:47

Remove warning from tests

37e9f1a

We need to do a fetch when a component calls the refresh

a6b912c

Removed unnecessary test

35c3621

This test is just validating what SC does. No need for it.

Address feedback

1c2ac7c

Swap to useTimeoutPoll given that it waits for the callback to be fin…

45688bc

…ished

PhilBastian requested a review from cquirosj October 14, 2025 05:37

johnsimons added 2 commits October 14, 2025 17:39

Changes based on review feedback

427b594

Merge pull request #2658 from Particular/john/refresh_v2

d307c09

cquirosj approved these changes Oct 14, 2025

View reviewed changes

johnsimons merged commit 73e78b3 into master Oct 14, 2025
5 checks passed

johnsimons deleted the john/flaky branch October 14, 2025 22:54

Uh oh!

Conversation

johnsimons commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnsimons commented Oct 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

johnsimons commented Sep 15, 2025 •

edited

Loading