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

Tests Frontend: Flaky ResolveForecastDialog test - handleCloseCalled is false #561

Closed
omarkohl opened this issue Jun 13, 2023 · 3 comments
Closed
Milestone

Comments

@omarkohl
Copy link
Contributor

omarkohl commented Jun 13, 2023

It failed once after about 320 executions and another time after about 60 executions of all frontend tests.

  ● forecast can be resolved with outcome

    expect(received).toBe(expected) // Object.is equality

    Expected: true
    Received: false

      171 |     await user.click(screen.getByRole("button", {name: "Save"}));
      172 |
    > 173 |     expect(handleCloseCalled).toBe(true);
          |                               ^
      174 |
      175 |     expect(requestBody).toBeTruthy();
      176 |     if (!requestBody) {

      at Object.<anonymous> (src/ResolveForecastDialog.test.tsx:173:31)

Probably a race condition when the dialog is closed the handleClose is not called immediately.

@omarkohl omarkohl converted this from a draft issue Jun 13, 2023
@omarkohl omarkohl changed the title Tests Frontend: Flaky ResolveForecastDialog test Tests Frontend: Flaky ResolveForecastDialog test - handleCloseCalled is false Jun 13, 2023
@omarkohl
Copy link
Contributor Author

omarkohl commented Jun 13, 2023

Script to run all tests until one fails.

#!/bin/bash
set -o errexit
set -o nounset
set -o pipefail

npm install

count=0

while true
do
    date
    echo $count
    # Seems to be the closest to what GitHub actions does
    npm run test -- --all --bail --ci --watchAll=false --runInBand || break
    echo
    echo
    echo
    sleep 2
    count=$((count+1))
done

omarkohl added a commit that referenced this issue Jun 13, 2023
Contrary to what I initially believed the issue does not seem to be in
the Footer.test.tsx but in App.test.tsx . The error message appears
after the Footer.test.tsx output and before the App.test.tsx output. The
App test includes all components but was not checking for the footer
content. My hypothesis is that when the test was done it tore down the
msw handlers making the fetch request for config.json made in the Footer
either abort or not be intercepted by msw. What set me on the right
track was the following issue, in particular this comment
jestjs/jest#9324 (comment) .

Before this fix the tests would fail about once every 10 to 40
executions. After this fix the tests failed due to #561 after about 320
and after about 60 executions. There were no more failures because of
this issue.
@omarkohl
Copy link
Contributor Author

Probably it's because the dialog only closes once the request to resolve the forecast has completed. Even in the test case with msw that might sometimes take a little longer.

With the following change the test immediately fails:

diff --git a/frontend/src/ResolveForecastDialog.test.tsx b/frontend/src/ResolveForecastDialog.test.tsx
index 8394aa0..e3ea2b9 100644
--- a/frontend/src/ResolveForecastDialog.test.tsx
+++ b/frontend/src/ResolveForecastDialog.test.tsx
@@ -80,6 +80,7 @@ test('forecast can be resolved with outcome', async () => {
     server.use(
         graphql.mutation("resolveForecast", async (req, res, ctx) => {
             requestBody = await req.json();
+            await new Promise(f => setTimeout(f, 1000));
             return res(
                 ctx.data({
                     "resolveForecast": {

@omarkohl omarkohl added this to the v0.4.0 milestone Jun 13, 2023
@omarkohl omarkohl moved this from Backlog to In Progress in Implementation Jun 13, 2023
omarkohl added a commit that referenced this issue Jun 15, 2023
The handleClose function is only called once the resolveForecast request
completes so there was a race condition where the handleClose was not
always called immediately after clicking 'Save' in the test.

Solve this by using waitFor() to repeat the verification several times.

Also replace the previous handleClose function with the
handleCloseCalled variable with the more elegant Jest mock functions.

To simulate real network conditions a small delay was left in the
response code.
omarkohl added a commit that referenced this issue Jun 15, 2023
This is to simulate real world network situation and flush out bugs such
as #290 and #561 earlier.
@omarkohl omarkohl moved this from In Progress to Done in Implementation Jun 15, 2023
@omarkohl
Copy link
Contributor Author

After commit 8f34ec4 about 600 executions of all tests were successful, so it seems reasonable to conclude that this has been resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

1 participant