chore: add Node 20 to CI #652
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
ooo interesting - test failure on 20: https://github.com/digidem/mapeo-core-next/actions/runs/9099775156/job/25013115175?pr=652#step:6:3694 # ok 2 - device info written to projects \# time = 359.825798ms
# \# device info sent to peers
# ok 1 - should be equal
# ok 2 - should be equal
# not ok 3 - should be equal
# ---
# actual: device0
# expected: new name
# operator: is
# source: |
# const [peersFromEvent] = await once(manager, 'local-peers')
# t.is(peersFromEvent.find(isChangedPeer)?.name, 'new name')
# --------^
#
# const updatedLocalPeers = await manager.listLocalPeers()
# stack: |
# ./test-e2e/device-info.js:167:9
# process.processTicksAndRejections (node:internal/process/task_queues:95:5)
# async ./test-e2e/device-info.js:176:3
# async Test._run (./node_modules/brittle/index.js:576:7)
# ...
# ok 4 - should be equal |
EvanHahn
requested changes
May 15, 2024
|
haven't been able to reproduce the test failure locally yet 🙃 |
|
I don't have a solution but I reproduced it on my machine with no issue. I wonder if it's a Linux thing?
|
Failure seems to occur on all machines in CI though? |
|
Ah true. Uh...did you run |
yep, no luck though 🤔 |
|
Strange...let me know if I can help at all.
|
if you can just...fix everything, that would be a great help 👍 |
|
I have a simple fix but don't really understand why it works. Investigating further...
|
[When peers connect, they exchange device info.][0] If peers make
redundant connections[^1], they can send redundant device info messages.
[In tests, we connect all managers to each other, which includes
redundant connections.][2] As you might expect, this might exchange
redundant device info messages.
This test was failing because of the following sequence of events:
1. Manager A starts connecting to Manager B.
2. Manager B starts connecting to Manager A.
3. The first connection (from event 1) is established and Manager A
sends its device info.
4. Manager B receives the device info.
5. In the test, `waitForPeers(managers, { waitForDeviceInfo: true })`
resolves because Manager B has received device info.
6. The second connection (from event 2) is established and Manager A
resends its device info.
7. In the test, Manager A changes its name and sends its device info
again.
8. Manager B receives the second device info.
9. Manager B receives the final device info.
The penultimate step was the problem. Once `waitForPeers()` resolved,
our test expected that no more device info messages were in flight, but
that wasn't true.
The failure was intermittent but its failure became more likely when
testing on Node 20.[^2]
[^1]: This can happen if two peers discover each other at the same time
and both connect to each other. This is swiftly resolved, but not before
the redundant device info messages are sent.
[^2]: I don't know why.
[0]: https://github.com/digidem/mapeo-core-next/blob/2eda1269ee8287199c9d9d1ec0ad30a98fad100c/src/mapeo-manager.js#L269
[2]: https://github.com/digidem/mapeo-core-next/blob/2eda1269ee8287199c9d9d1ec0ad30a98fad100c/test-e2e/utils.js#L46-L53
|
Pushed eb23f66 to fix the problem. It has a wordy explanation about the race condition that happened here. |
EvanHahn
approved these changes
May 20, 2024
EvanHahn
added a commit
that referenced
this pull request
May 20, 2024
Co-authored-by: Evan Hahn <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds Node 20 to the matrix for CI, as that's the current LTS and it would seem advisable to be testing on that version in addition to 18 (which is still needed because of our usage of NodeJS Mobile)