Unit test some worker protocol serializers#8923
Conversation
Ericson2314
requested review from
edolstra,
fricklerhandwerk and
thufschmitt
as code owners
github-actions
bot
added
the
with-tests
thufschmitt
left a comment
thufschmitt
left a comment
There was a problem hiding this comment.
That's pretty nice :) I expect that test failures will be rather awful since we don't have a generic way to pretty-print the protocol, but at least we'll have some proper failures.
My main issue right now is with the documentation for these, which isn't really understandable without already knowing what's going on
| You can run the whole testsuite with `make check`, or the tests for a specific component with `make libfoo-tests_RUN`. | ||
| Finer-grained filtering is also possible using the [--gtest_filter](https://google.github.io/googletest/advanced.html#running-a-subset-of-the-tests) command-line option, or the `GTEST_FILTER` environment variable. | ||
|
|
||
| ### Characterization testing |
There was a problem hiding this comment.
I gather this is the same as “Golden tests”, with the latter being (as far as I know) much more common. Maybe rename it?
There was a problem hiding this comment.
I went with that name before seen it was the one Wikipedia went with. See what I have since linked "below" too.
|
|
||
| ### Characterization testing | ||
|
|
||
| See below for a broder discussion. |
There was a problem hiding this comment.
There was an existing long section on characterization testing, but within the functional tests. I am not sure what to put here, but I figured I shouldn't just repeat the same information. Certainly could be better! (e.g. maybe since this comes first most of the information should be moved here, and the other section should reference it.)
| ### Characterization testing | ||
|
|
||
| See below for a broder discussion. | ||
| `_NIX_TEST_ACCEPT=1` is also used. | ||
| The data is in `unit-test-data`; | ||
| the path to that directory is passed to the unit test executable with the environment variable `UNIT_TEST_ENV`. | ||
|
|
There was a problem hiding this comment.
I don't really have a clue as to what I should take out of this paragraph. What I'd expect to know is
- How do I run these tests?
- How do I edit one or add a new one?
- How do I regenerate the golden files?
There was a problem hiding this comment.
1 and 3 are now covered. (They are just unit tests, they are run like any other units tests). 2 I am not sure it is good to include yet because there is no general pattern yet.
I would like to convert a bunch of existing unit tests to use this sort of thing down the road, and at that point I think we'll have enough generalized infra to answer this question properly.
|
As a “would be good for later” item, how about keeping the old golden records forever and test that they still read properly? That would give us some extra guaranties on the backwards-compatibility of the protocol |
Ericson2314
force-pushed
the
test-proto
branch
2 times, most recently
from
47571b9 to
645f184
Compare
|
OK, Added some more documentation. Some of the language is taken from #8886, but not the actual moving things around.
Yeah the refactor of mine by adding the version info to |
Ericson2314
force-pushed
the
test-proto
branch
from
645f184 to
e631f36
Compare
Continue with the characterization testing idioms begun in c704844, but this time for unit tests. Co-authored-by: Andreas Rammhold <[email protected]>
Ericson2314
force-pushed
the
test-proto
branch
from
e631f36 to
7ff4343
Compare
| > **Note** | ||
| > Due to the way googletest works, downstream unit test executables will actually include and re-run upstream library tests. | ||
| > Therefore it is important that the same value for `_NIX_TEST_UNIT_DATA` be used with the tests for each library. | ||
| > That is why we have the test data nested within a single `unit-test-data` directory. |
There was a problem hiding this comment.
Would it be better to solve this by creating test support libraries, which don't include the actual tests?
There was a problem hiding this comment.
Can we store them in hexdump -C format?
This will make changes easier to review.
00000000 40 00 00 00 00 00 00 00 74 65 78 74 3a 73 68 61 |@.......text:sha|
00000010 32 35 36 3a 31 76 62 32 78 64 36 37 37 35 30 6a |256:1vb2xd67750j|
00000020 72 76 71 67 79 6b 78 72 66 35 69 70 6c 33 68 31 |rvqgykxrf5ipl3h1|
00000030 71 35 37 73 78 63 35 37 71 35 6e 7a 6a 77 35 36 |q57sxc57q5nzjw56|
00000040 62 71 70 33 73 78 7a 72 2b 00 00 00 00 00 00 00 |bqp3sxzr+.......|
00000050 66 69 78 65 64 3a 73 68 61 31 3a 31 61 6d 6b 36 |fixed:sha1:1amk6|
00000060 6e 37 77 78 6a 6c 77 34 71 6b 71 36 64 63 6a 36 |n7wxjlw4qkq6dcj6|
00000070 6d 6d 37 68 77 33 61 63 72 77 30 00 00 00 00 00 |mm7hw3acrw0.....|
00000080 43 00 00 00 00 00 00 00 66 69 78 65 64 3a 72 3a |C.......fixed:r:|
00000090 73 68 61 32 35 36 3a 31 6c 72 31 38 37 76 36 64 |sha256:1lr187v6d|
000000a0 63 6b 31 72 6a 68 32 6a 36 73 76 70 69 6b 63 66 |ck1rjh2j6svpikcf|
000000b0 7a 35 33 77 79 6c 33 71 72 6c 63 62 62 34 30 35 |z53wyl3qrlcbb405|
000000c0 7a 6c 68 31 33 78 30 6b 68 68 68 00 00 00 00 00 |zlh13x0khhh.....|
000000d0
There was a problem hiding this comment.
Or this, which is more diff friendly
$ hexdump -e '8 1 "%02x "' -e '" | "' -e '8 "%_p"' -e '"\n"' unit-test-data/libstore/worker-protocol/content-address.bin
40 00 00 00 00 00 00 00 | @.......
74 65 78 74 3a 73 68 61 | text:sha
32 35 36 3a 31 76 62 32 | 256:1vb2
78 64 36 37 37 35 30 6a | xd67750j
72 76 71 67 79 6b 78 72 | rvqgykxr
66 35 69 70 6c 33 68 31 | f5ipl3h1
71 35 37 73 78 63 35 37 | q57sxc57
71 35 6e 7a 6a 77 35 36 | q5nzjw56
62 71 70 33 73 78 7a 72 | bqp3sxzr
2b 00 00 00 00 00 00 00 | +.......
66 69 78 65 64 3a 73 68 | fixed:sh
61 31 3a 31 61 6d 6b 36 | a1:1amk6
6e 37 77 78 6a 6c 77 34 | n7wxjlw4
71 6b 71 36 64 63 6a 36 | qkq6dcj6
6d 6d 37 68 77 33 61 63 | mm7hw3ac
72 77 30 00 00 00 00 00 | rw0.....
43 00 00 00 00 00 00 00 | C.......
66 69 78 65 64 3a 72 3a | fixed:r:
73 68 61 32 35 36 3a 31 | sha256:1
6c 72 31 38 37 76 36 64 | lr187v6d
63 6b 31 72 6a 68 32 6a | ck1rjh2j
36 73 76 70 69 6b 63 66 | 6svpikcf
7a 35 33 77 79 6c 33 71 | z53wyl3q
72 6c 63 62 62 34 30 35 | rlcbb405
7a 6c 68 31 33 78 30 6b | zlh13x0k
68 68 68 00 00 00 00 00 | hhh.....There was a problem hiding this comment.
Do you know how to "un-hexdump" files?
There was a problem hiding this comment.
Do you know how to "un-hexdump" files?
Do we need to? We can just diff the hex dumps, right?
There was a problem hiding this comment.
We have tests that read the dump and compare the result to something in the code; we want to test both the read and write directions.
There was a problem hiding this comment.
Duh, of course 🤦♂️
xxd can do the reverse (at least it can reverse itself, dunno about the exact format spit out by hexdump). But I've no idea whether that's accessible as a library, and it's not available in the build env yet (though it's provided by busybox, so not a huge dependency).
So we can probably save that for a follow-up, unless someone wants to re-implement a hexdump in C++ (which probably isn't hard, but still)
There was a problem hiding this comment.
Yes I would prefer follow up. (Generally I hope these never change, rather we add new versions in new files, FWIW.)
Anything else before this can be merged?
Unit test some worker protocol serializers (cherry picked from commit c6faef6) Change-Id: I99e36f5f17eb7642211a4e42a16b143424f164b4
4 participants
Motivation
Context
Continue with the characterization testing idioms begun in c704844, but this time for unit tests.
Checklist for maintainers
Maintainers: tick if completed or explain if not relevant
tests/**.shsrc/*/teststests/nixos/*Priorities
Add 👍 to pull requests you find important.