+ add a test for parseAndRemoveLogLevel()
+ add a test for drainPendingMessages()
+ test JSON log encoding including several special cases
Other tests frequently send logs but a) don't check the result and
b) do so by happenstance, such that the code in encode() was not
consistently being exercised and leading to spurious changes in
code coverage. These tests attempt to more systematically test
the logging function.
This is the second attempt to add these tests, the first attempt
(in https://github.com/tailscale/tailscale/pull/1114) had two issues:
1. httptest.NewServer creates multiple goroutine handlers, and
logtail uses goroutines to upload, but the first version had no
locking in the server to guard this.
Moved data handling into channels to get synchronization.
2. The channel to notify the test of the arrival of data had a depth
of 1, in cases where the Logger sent multiple uploads it would
block the server.
This resulted in the first iteration of these tests being flaky,
and we reverted it.
This new version of the tests has passed with
go test -race -count=10000
and seems solid.
Signed-off-by: Denton Gentry <dgentry@tailscale.com>
This test serves two purposes:
+ check that Write() returns an error if the tstun has been
closed.
+ ensure that the close-related code in tstun is exercised in
a test case. We were getting spurious code coverage adds/drops
based on timing of when the test case finished.
Signed-off-by: Denton Gentry <dgentry@tailscale.com>
This reverts commit e4f53e9b6f.
At least two of these tests are flakey, reverting until they can be
made more robust.
Signed-off-by: Denton Gentry <dgentry@tailscale.com>
* logtail: test parseAndRemoveLogLevel()
Signed-off-by: Denton Gentry <dgentry@tailscale.com>
* logtail: test JSON log encoding.
Expand TestUploadMessages to also exercise the encoding functions
in logtail, like JSON logging and timestamps.
Other tests frequently send logs but a) don't check the result and
b) do so by happenstance, such that the lines in encode() were not
consistently being exercised and leading to spurious changes in
code coverage.
Signed-off-by: Denton Gentry <dgentry@tailscale.com>
* logtail: add a test for drainPendingMessages
Make the client buffer some messages before the upload server
becomes available.
Signed-off-by: Denton Gentry <dgentry@tailscale.com>
* logtail: use %q, raw strings, and io.WriteString
%q escapes binary characters for us.
raw strings avoid so much backslash escaping
Signed-off-by: Denton Gentry <dgentry@tailscale.com>
Right now TestFastShutdown tries to upload logs to localhost:1234,
which will most likely respond with an error. However if one has an
actual service running on port 1234, it would receive a connection
attempting to POST every time the unit test runs.
Start a local server and direct the upload there instead.
Signed-off-by: Denton Gentry <dgentry@tailscale.com>
In sendDiscoMessage there is a check of whether the connection is
closed, which is not being reliably exercised by other tests.
This shows up in code coverage reports, the lines of code in
sendDiscoMessage are alternately added and subtracted from
code coverage.
Add a test to specifically exercise and verify this code path.
Signed-off-by: Denton Gentry <dgentry@tailscale.com>
Start an HTTP server to accept POST requests, and upload some logs to
it. Check that uploaded logs were received.
Code in logtail:drainPending was not being reliably exercised by other
tests. This shows up in code coverage reports, as lines of code in
drainPending are alternately added and subtracted from code coverage.
This test will reliably exercise and verify this code.
Signed-off-by: Denton Gentry <dgentry@tailscale.com>
All cases in lessThan are not reliably exercised by other tests.
This shows up in code coverage metrics as lines in lessThan are
alternately added and removed from coverage.
Add a test case to systematically test all conditions.
Signed-off-by: Denton Gentry <dgentry@tailscale.com>
In derpWriteChanOfAddr when we call derphttp.NewRegionClient(),
there is a check of whether the connection is already errored and
if so it returns before grabbing the lock. The lock might already
be held and would be a deadlock.
This corner case is not being reliably exercised by other tests.
This shows up in code coverage reports, the lines of code in
derpWriteChanOfAddr are alternately added and subtracted from
code coverage.
Add a test to specifically exercise this code path, and verify that
it doesn't deadlock.
This is the best tradeoff I could come up with:
+ the moment code calls Err() to check if there is an error, we
grab the lock to make sure it would deadlock if it tries to grab
the lock itself.
+ if a new call to Err() is added in this code path, only the
first one will be covered and the rest will not be tested.
+ this test doesn't verify whether code is checking for Err() in
the right place, which ideally I guess it would.
Signed-off-by: Denton Gentry <dgentry@tailscale.com>
To be honest I'm not fond of Golden Bytes tests like this, but
not so much as to want to rewrite the whole test. The DNS byte
format is essentially immutable at this point, the encoded bytes
aren't going to change. The rest of the test assumptions about
hostnames might, but we can fix that when it comes.
Signed-off-by: Denton Gentry <dgentry@tailscale.com>
+ we don't need an exactly accurate count of the number of times each
time ran. Remove -covermode, the default "set" will be fine to just
track whether a given line ran at all.
+ add -benchtime=1x. We only need to run the benchmarks once.
+ -bench=. to match any character.
Signed-off-by: Denton Gentry <dgentry@tailscale.com>
We include -bench because some parts of the codebase, like
smallzstd, do not have regular unit tests but do have very
good benchmark tests that covers all functions.
Signed-off-by: Denton Gentry <dgentry@tailscale.com>
