Factor out bits of the worker protocol to use elsewhere #9099
Merged
Ericson2314 merged 2 commits intoNixOS:masterfrom Oct 9, 2023
Merged
Factor out bits of the worker protocol to use elsewhere #9099Ericson2314 merged 2 commits intoNixOS:masterfrom
Ericson2314 merged 2 commits intoNixOS:masterfrom
Conversation
Ericson2314
requested review from
edolstra,
fricklerhandwerk and
thufschmitt
as code owners
Ericson2314
added
the
protocol
github-actions
bot
added
with-tests
Ericson2314
added
the
protocol
Member
Author
|
https://github.com/google/googletest/blob/main/docs/advanced.md this claims there is a way to reuse whole testsuites across implementations, but it didn't immediately work for me and also it seems quite weird. |
Ericson2314
force-pushed
the
common-proto
branch
from
dccf8c7 to
cd4b9c0
Compare
This introduces some shared infrastructure for our notion of protocols. We can then define multiple protocols in terms of that notion. We an also express how particular protocols depend on each other. For example, we can define a common protocol and a worker protocol, where the second depends on the first in terms of the data types it can read and write. The "serve" protocol can just use the common one for now, but will eventually need its own machinary just like the worker protocol for version-aware serialisers
Copy the relevant tests to ensure the new interfaces added in the last commit are tested. Perhaps I should try to deduplicat these tests some more. However its not clear how to do that outside of a big ugly C++ macro. https://github.com/google/googletest/blob/main/docs/advanced.md has some stuff but it is cumbersome and I didn't figure it out yet. This is done in a separate commit in order to be sure that the first commit really didn't change any behavior; if we changed the implementation and the tests at once, it would be harder to tell whether or not some behavioral changes slipped in what is supposed to be a "pure refactor". Co-Authored-By: Valentin Gagarin <valentin.gagarin@tweag.io>
Ericson2314
force-pushed
the
common-proto
branch
from
cd4b9c0 to
4de54b2
Compare
fricklerhandwerk
approved these changes
Oct 9, 2023
Contributor
fricklerhandwerk
left a comment
fricklerhandwerk
left a comment
There was a problem hiding this comment.
Reviewed in detail and ran the tests locally.
tebowy
pushed a commit
to tebowy/nix
that referenced
this pull request
Jul 11, 2024
Factor out bits of the worker protocol to use elsewhere (cherry picked from commit 4b1a973) Change-Id: If93afa0f8b1cf9b0e705b34fa71e6fd708752758
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Motivation
This introduces some shared infrastructure for our notion of protocols.
We can then define multiple protocols in terms of that notion.
We an also express how particular protocols depend on each other.
For example, we can define a common protocol and a worker protocol,
where the second depends on the first in terms of the data types it can
read and write.
The "serve" protocol can just use the common one for now, but will
eventually need its own machinary just like the worker protocol for
version-aware serialisers. Even though all the commits of that first PR are approved, I think it is good to split it up when adding tests for extra clarity.
Context
The first commit of this is the first commit of #6223, a PR which was approved except for needing tests. The second commit of this is new, and adds such missing tests.
Priorities
Add 👍 to pull requests you find important.