Daphne integration test. #346
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
branlwyd
force-pushed
the
bran/daphne-integration-test
branch
4 times, most recently
from
9b9fc86 to
9c42731
Compare
Also, refactor the existing Janus-only integration test to use the same code as the new Daphne-Janus integration tests. The hermetic Daphne instances are implemented by including a compiled version of Daphne as an artifact, and running it inside miniflare, based heavily on the manual instructions in the Daphne repository. Miniflare is now a required dependency for running tests; instructions for running tests & updating the version of Daphne used for tests is included in the README.md. Ideally, we would start a container, but Daphne does not yet build out a suitable container.
branlwyd
force-pushed
the
bran/daphne-integration-test
branch
from
9c42731 to
52fbc76
Compare
divergentdave
approved these changes
Aug 1, 2022
|
I think we could use a tracing subscriber in each of the new test functions as well. |
This is evidently required on some workstations/OS'es. It's not required on mine, but it also doesn't cause problems. Also, fixup a stale comment.
Prior to this commit, the miniflare instance running Daphne hermetic test instances would listen for connections on real network interfaces. Now, we only listen for connections on loopback, matching the behavior of the Janus hemetic test instances.
This works around an issue in Daphne where some measurements will not be included in the final aggregate when Daphne is in the leader position, causing the final aggregated value to be incorrect. I work around this instead of disabling the test in order to show that most of the functionality still works.
|
N.B. we are currently working around the Daphne issue that made the test flaky. We work around the issue instead of disabling the test in order to maintain that most of the functionality (i.e. the protocol back-and-forth) still works. |
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.
Also, refactor the existing Janus-only integration test to use the same
code as the new Daphne-Janus integration tests.
The hermetic Daphne instances are implemented by including a compiled
version of Daphne as an artifact, and running it inside miniflare, based
heavily on the manual instructions in the Daphne repository. Miniflare
is now a required dependency for running tests; instructions for running
tests & updating the version of Daphne used for tests is included in the
README.md. Ideally, we would start a container, but Daphne does not yet
build out a suitable container.
My next steps will be chatting with the Daphne folks to see if I can
contribute most of this logic to Daphne itself, which would be a more
maintainable arrangement.
The
daphne_janustest is currently flaky; I believe that this revealsa real functional issue, but I'm going to dig in separately from this
PR.
Fixes #341.