Make SSH integration tests easier to debug #1046
Open
Conversation
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
adombeck
force-pushed
the
improve-ssh-tests
branch
from
a9453fd to
cda8f9b
Compare
adombeck
force-pushed
the
improve-ssh-tests
branch
11 times, most recently
from
7762549 to
28f1faa
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1046 +/- ##
==========================================
- Coverage 88.04% 87.74% -0.30%
==========================================
Files 85 87 +2
Lines 6039 6112 +73
Branches 111 111
==========================================
+ Hits 5317 5363 +46
- Misses 666 693 +27
Partials 56 56 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
adombeck
force-pushed
the
improve-ssh-tests
branch
16 times, most recently
from
f1d6e86 to
a0718d1
Compare
To avoid having to pass it to each method call.
We never use more than one output, so let's simplify things a bit.
It's a bit hidden in there. Let's move it to the caller so we don't wonder in the caller whether the tape is saved as an artifact or not.
When running sshd in daemon mode, the child processes it spawns for handling ssh connections do not print logs to the log file, and their stderr is not connected, so the log messages are lost. By running sshd in the foreground via -D, and making it log to stderr via -e, we get both the main processes and all child processes logs printed to stderr.
The service name is used as the filename of the artifact, where "authd-cli" doesn't make it clear that this is a PAM service file.
... but only print debug1 messages to stderr, to avoid spamming the test output too much. This can be configured via the AUTHD_SSHD_STDERR_DEBUG_LEVEL environment variable.
We were building everything twice, because testSSHAuthenticate is called twice (once with sharedSSHD set to true and once set to false).
We were printing "Building PAM module" when we were building the sshd_preloader library.
Output written directly to stdout/stderr is sometimes assigned to the wrong test, even when using `go test -json`. Using `t.Log` instead should avoid the problem.
This makes our own TestWriter obsolete. Also has the effect that the output written via this is not prefixed with the file and line, which makes it a lot more readable.
And use it when running gcov, because its output is a bit verbose and unhelpful when there is no error.
It's building a shared library.
I've seen this test time out in the CI.
So that we're able to use them from other packages than the pam/integration-tests.
Writing both stdout and stderr to the same file causes a data race. Lets use our SyncBuffer instead.
It failed with that error in CI:
ssh_test.go:491:
Error Trace: /home/runner/work/authd/authd/pam/integration-tests/ssh_test.go:795
/home/runner/work/authd/authd/pam/integration-tests/ssh_test.go:659
/home/runner/work/authd/authd/pam/integration-tests/ssh_test.go:491
/home/runner/work/authd/authd/internal/testutils/testrun.go:83
Error: Received unexpected error:
dial tcp :35523: connect: connection reset by peer
Test: TestSSHAuthenticate/Authenticate_user_switching_auth_mode
The tool's spelling is VHS, not vhs, so let's use that.
The function name RunAuthd made it seem like it would block until authd has finished, which is not the case. StartAuthd makes it clear that the function returns once authd was started.
adombeck
force-pushed
the
improve-ssh-tests
branch
from
17b7462 to
042e6c2
Compare
adombeck
changed the title
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.
Changes for improved debuggability, test execution time, and maintainability, including:
UDENG-8137