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: invite helper should automatically wait for peers #865

Merged
merged 2 commits into from
Oct 9, 2024

Conversation

EvanHahn
Copy link
Contributor

@EvanHahn EvanHahn commented Sep 24, 2024

I recommend reviewing this with whitespace changes disabled.

Our invite helper breaks if other peers aren't found. There's no time we'd use invite without waiting for other peers to be found, so let's always wait for them in invite.

@EvanHahn EvanHahn force-pushed the invite-should-wait-for-peers branch from 3472ca3 to d5f2b43 Compare September 24, 2024 18:55
EvanHahn added a commit that referenced this pull request Sep 24, 2024
`waitForPeers`, a test helper, currently checks that the *number* of
connected peers is expected. This has two problems:

- If the number of connected peers is *more than needed*, it will never
  resolve.
- If the *number* of connected peers is correct but the actual peers are
  different, the promise could resolve prematurely.

This snippet highlights the problem:

```javascript
const managers = await createManagers(3, t)
const [a, b, c] = managers

connectPeers(managers)

await waitForPeers([a, c])
```

I think this is a useful change on its own, but will also make [an
upcoming change][0] easier.

[0]: #865
EvanHahn added a commit that referenced this pull request Sep 25, 2024
`waitForPeers`, a test helper, currently checks that the *number* of
connected peers is expected. This has two problems:

- If the number of connected peers is *more than needed*, it will never
  resolve.
- If the *number* of connected peers is correct but the actual peers are
  different, the promise could resolve prematurely.

This snippet highlights the problem:

```javascript
const managers = await createManagers(3, t)
const [a, b, c] = managers

connectPeers(managers)

await waitForPeers([a, c])
```

I think this is a useful change on its own, but will also make [an
upcoming change][0] easier.

[0]: #865
Base automatically changed from fix-test-invite-race-conditions to main September 26, 2024 16:57
@EvanHahn EvanHahn force-pushed the invite-should-wait-for-peers branch 2 times, most recently from 5f0dff7 to c07a8a8 Compare September 26, 2024 19:42
@EvanHahn EvanHahn marked this pull request as ready for review September 26, 2024 19:45
Our `invite` helper breaks if other peers aren't found. There's no time
we'd use `invite` without waiting for other peers to be found, so let's
always wait for them in `invite`.
@EvanHahn EvanHahn force-pushed the invite-should-wait-for-peers branch from c07a8a8 to 051085a Compare October 1, 2024 22:59
@EvanHahn EvanHahn merged commit 8140098 into main Oct 9, 2024
6 checks passed
@EvanHahn EvanHahn deleted the invite-should-wait-for-peers branch October 9, 2024 14:50
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