Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sensors tests improvements #237

Merged
merged 5 commits into from
Jul 21, 2022
Merged

Conversation

kkourt
Copy link
Contributor

@kkourt kkourt commented Jul 15, 2022

Improve the sensor tests, mostly with respect to logging.

See commits.

@kkourt kkourt requested a review from a team as a code owner July 15, 2022 14:24
@kkourt kkourt requested a review from willfindlay July 15, 2022 14:24
@kkourt kkourt force-pushed the pr/kkourt/sensors-tests-improvements branch 2 times, most recently from 6caf260 to cb96700 Compare July 15, 2022 14:41
Copy link
Contributor

@willfindlay willfindlay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you!

@kkourt kkourt added the needs-rebase This PR needs to be rebased because it has merge conflicts. label Jul 21, 2022
kkourt added 5 commits July 21, 2022 12:41
Turns out that captureLog operated on the default (global) logrus logger
and never actually redirected output to t.Logf.  Fix this, here, and
also add the logger to the output so that we can redirect the output
back (even though it's not obvious to me why we need to do so).

There is still some output from the observer (which has its own logrus
instance). See subsequent commits.

Signed-off-by: Kornilios Kourtis <[email protected]>
Now we can use it outside of jsonChecker.

Signed-off-by: Kornilios Kourtis <[email protected]>
There was duplicated code among the sensor tests. This PR creates a
pkg/testutils/sensors package and places the code there.

This package includes:
 - a TestSensorsRun function. This must be called by all sensor tests in
   their TestMain.
 - part of the above function is to setup some configuration (e.g., via
   flags. We add a helper function so that this confiugration can be
   accessed by the tests.

There are additional things that can be pushed in the above package to
simplify the tests, but this is beyond the scope of this patch.

Signed-off-by: Kornilios Kourtis <[email protected]>
This is an quick optimization for tests that expect failure.

It improves the performance of a successful test by 33%:
before:
 ok      github.com/cilium/tetragon/pkg/sensors/tracing  331.502s
now:
 ok      github.com/cilium/tetragon/pkg/sensors/tracing  221.583s

Signed-off-by: Kornilios Kourtis <[email protected]>
Before this patch, we redirected the log when testing sensors only in
the json checker. This, however, meant that a lot of log messages from
the observer will still be part of the output.

Fix this by redirecting the log output in the initialization of the
testing observer.

While we are at it, add flag to competely disable tetragon log output.

Example:
$ sudo sh -c 'CGO_LDFLAGS=-L$(realpath ./lib)  LD_LIBRARY_PATH=$(realpath ./lib) go test -failfast ./pkg/sensors/test -test.v -test.run . -disable-tetragon-logs'
=== RUN   TestTestChecker
    jsonchecker.go:132: jsonTestCheck: opening: /tmp/tetragon.gotest.TestTestChecker.812556859.json
    jsonchecker.go:126: test failed, marking export file to be kept
    checker_test.go:57: got error: dummy error
    filenames.go:60: deleting export file for TestTestChecker (/tmp/tetragon.gotest.TestTestChecker.812556859.json)
--- PASS: TestTestChecker (1.28s)
=== RUN   TestSensorLseekLoad
    jsonchecker.go:132: jsonTestCheck: opening: /tmp/tetragon.gotest.TestSensorLseekLoad.3782420253.json
    filenames.go:60: deleting export file for TestSensorLseekLoad (/tmp/tetragon.gotest.TestSensorLseekLoad.3782420253.json)
--- PASS: TestSensorLseekLoad (1.11s)
=== RUN   TestSensorLseekEnable
    jsonchecker.go:132: jsonTestCheck: opening: /tmp/tetragon.gotest.TestSensorLseekEnable.985908057.json
    filenames.go:60: deleting export file for TestSensorLseekEnable (/tmp/tetragon.gotest.TestSensorLseekEnable.985908057.json)
--- PASS: TestSensorLseekEnable (1.19s)
PASS
ok      github.com/cilium/tetragon/pkg/sensors/test     3.629s

Signed-off-by: Kornilios Kourtis <[email protected]>
@kkourt kkourt force-pushed the pr/kkourt/sensors-tests-improvements branch from cb96700 to a82015f Compare July 21, 2022 10:55
@kkourt kkourt merged commit 979a7b8 into main Jul 21, 2022
@kkourt kkourt deleted the pr/kkourt/sensors-tests-improvements branch July 21, 2022 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase This PR needs to be rebased because it has merge conflicts.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants