Skip to content
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

test(queryClient): Adjust performance threshholds #6937

Closed
wants to merge 3 commits into from

Conversation

SebKranz
Copy link
Contributor

@SebKranz SebKranz commented Feb 19, 2024

The pipeline was found to sometimes exceed the threshold of 500ms. Since I expect the the required time to increase linearly, increase the row count by *1.5 and quadruple the time-threshhold.

Local Tests on my Machine

Screenshot 2024-02-19 at 14 06 54

(yellow is prior to #6918, red is after, all times in ms)

Copy link

vercel bot commented Feb 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
query ⬜️ Ignored (Inspect) Visit Preview Feb 19, 2024 7:45pm

Copy link

nx-cloud bot commented Feb 19, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit bc6e65e. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

Copy link

codesandbox-ci bot commented Feb 19, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit bc6e65e:

Sandbox Source
@tanstack/query-example-angular-basic Configuration
@tanstack/query-example-react-basic-typescript Configuration
@tanstack/query-example-solid-basic-typescript Configuration
@tanstack/query-example-svelte-basic Configuration
@tanstack/query-example-vue-basic Configuration

@SebKranz
Copy link
Contributor Author

SebKranz commented Feb 19, 2024

Unfortunately, I'm a bit unsure whether the best approach would be to increase or decrease the threshold & query-count here.

Increasing will potentially slow down the pipeline whereas decreasing might lead to false negatives. 500ms felt like a good compromise but apparently wasn't lenient enough. (see #6918 (comment))

@milhamm do you have opinions on this?

@TkDodo
Copy link
Collaborator

TkDodo commented Feb 19, 2024

how about a test that tests if execution doesn't grow exponentially instead ?

@SebKranz
Copy link
Contributor Author

SebKranz commented Feb 19, 2024

how about a test that tests if execution doesn't grow exponentially instead ?

I fear we would run into similar flakiness, because the cpu-load might not be stable over the course of the test (eg see the variance in my local tests). Also, the tests would become more complex since it would need some kind of regression algorithm.

I would suggest the following plan for now:

  1. Try these new defaults and see if the problem occurs again
  2. If it does, enable retries in vitest like so:
    test(
        'should set 15k data in less than 2000ms',
        () => {
             // ...
        }
        { retry: 3 },
     )
  3. If that still fails, I'll consider an alternative testing approach.

@SebKranz SebKranz force-pushed the fix-flaky-performance-tests branch from 48715b7 to 9db4d89 Compare February 19, 2024 14:56
@TkDodo
Copy link
Collaborator

TkDodo commented Feb 19, 2024

we have retries in vitest enabled

@SebKranz SebKranz force-pushed the fix-flaky-performance-tests branch 3 times, most recently from 03ee7b4 to c86f4c5 Compare February 19, 2024 17:45
The pipeline was found to sometimes exceed the threshold of 500ms.
@SebKranz SebKranz force-pushed the fix-flaky-performance-tests branch from c86f4c5 to e70264b Compare February 19, 2024 17:49
@SebKranz SebKranz marked this pull request as draft February 19, 2024 18:01
@SebKranz
Copy link
Contributor Author

SebKranz commented Feb 19, 2024

There are some unrelated tests that depend on the event loop not being blocked (such as useIsMutating).
If the performance tests are scheduled at the same time as these tests, their timers execute delayed and fail the unrelated test.

Unless we run them sequentially, there's no feasible way of doing performance-testing IMO.

I'll check if there's a good way of marking specific tests for sequential execution in NX, but it might be best to just remove the performance tests for now.

@SebKranz SebKranz force-pushed the fix-flaky-performance-tests branch from d2e0b05 to acebdc4 Compare February 19, 2024 18:43
@SebKranz SebKranz force-pushed the fix-flaky-performance-tests branch from 174834b to bc6e65e Compare February 19, 2024 19:45
@SebKranz
Copy link
Contributor Author

I've created a PR to remove these tests: #6946

@SebKranz SebKranz closed this Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants