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

The queueToDevice.spec.ts test suite is flaky #2561

Closed
robintown opened this issue Aug 3, 2022 · 2 comments · Fixed by #2841
Closed

The queueToDevice.spec.ts test suite is flaky #2561

robintown opened this issue Aug 3, 2022 · 2 comments · Fixed by #2841
Assignees
Labels
A-Testing Testing, code coverage, etc. T-Defect Z-Flaky-Test A test is raising false alarms

Comments

@robintown
Copy link
Member

Me:

We seem to have an issue in our js-sdk tests somewhere with the new to-device message queue:

  ●  Cannot log after tests are done. Did you forget to wait for something async in your test?
    Attempted to log "All queued to-device messages sent".

      50 |             return console[methodName](...args);
      51 |         } else {
    > 52 |             return console.log(...args);
         |                            ^
      53 |         }
      54 |         /* eslint-enable no-console */
      55 |     };

      at console.log (node_modules/@jest/console/build/BufferedConsole.js:187:10)
      at Logger.log (src/logger.ts:52:28)
      at ToDeviceMessageQueue.debug [as sendQueue] (src/ToDeviceMessageQueue.ts:84:20)

This is coming out of the CI which suppresses most output so I can't see what test suite is causing it, and I can't reproduce it locally either. So, I guess this is a note to shout if anyone runs into it locally.

Travis:

image(1)
I get different failures as a flake 😛
Expected 1 got 0 on a flush for the first one. Expected 20 got 1 on the object size for the second.

@robintown robintown added A-Testing Testing, code coverage, etc. Z-Flaky-Test A test is raising false alarms labels Aug 4, 2022
@dbkr
Copy link
Member

dbkr commented Aug 5, 2022

I've increased the timeout on the flush in the above PR, but I can't repro the failure so I'm struggling to see how it would be possible for the code to only send one message in the request. It looks like mock-request does flush the requests in order so I don't think it's that the requests are getting mixed up.

@robintown
Copy link
Member Author

I still got it to fail, even with the new timeout in place:

Summary of all failing tests
 FAIL  spec/unit/queueToDevice.spec.ts
  ● queueToDevice (IndexedDB store) › retries when a message is retried

    expect(received).toEqual(expected) // deep equality

    Expected: 1
    Received: 0

      301 |         });
      302 |
    > 303 |         expect(await httpBackend.flush(null, 1, 1)).toEqual(1);
          |                                                     ^
      304 |         await flushPromises();
      305 |
      306 |         const dummyEvent = new MatrixEvent({

      at Object.toEqual (spec/unit/queueToDevice.spec.ts:303:53)

  ● queueToDevice (IndexedDB store) › splits many messages into multiple HTTP requests

    expect(received).toEqual(expected) // deep equality

    Expected: 20
    Received: 1

      332 |             "PUT", "/sendToDevice/org.example.foo/",
      333 |         ).check((request) => {
    > 334 |             expect(Object.keys(request.data.messages).length).toEqual(20);
          |                                                               ^
      335 |         }).respond(200, {});
      336 |
      337 |         httpBackend.when(

      at Array.toEqual (spec/unit/queueToDevice.spec.ts:334:63)
      at HttpBackend._takeFromQueue (node_modules/matrix-mock-request/lib/index.js:217:34)
      at _tryFlush (node_modules/matrix-mock-request/lib/index.js:102:20)
      at Timeout.tryFlush [as _onTimeout] (node_modules/matrix-mock-request/lib/index.js:92:13)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Testing Testing, code coverage, etc. T-Defect Z-Flaky-Test A test is raising false alarms
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants