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

tests: introduce end-to-end test framework #226

Merged
merged 2 commits into from
Jul 28, 2022
Merged

Conversation

willfindlay
Copy link
Contributor

This PR introduces a new end-to-end test framework in tests/e2e which we can use to
write cross-provider end-to-end tests for Tetragon. The tests themselves live in
tests/e2e/tests. I have added a skeleton file with lots of comments that folks can use
as a starting point for writing new tests.

Updated the Makefile to support e2e-tests by adding a separate e2e-test target and making
sure that regular make test will not call into the e2e-tests by pointing it directly at
./pkg/.... Also added an EXTRA_TESTFLAGS variable to the Makefile that can be used to
pass flags directly to the test binary from either a make test or make e2e-test.

The plan is to use this new framework to start writing e2e tests for Tetragon features.

@willfindlay willfindlay requested a review from a team as a code owner July 11, 2022 16:26
@willfindlay willfindlay requested review from kevsecurity, tixxdz, kkourt and jrfastab and removed request for kevsecurity July 11, 2022 16:26
@willfindlay willfindlay force-pushed the pr/willfindlay/e2e-framework branch 2 times, most recently from 158ff4a to dbcfacb Compare July 11, 2022 17:53
@willfindlay willfindlay force-pushed the pr/willfindlay/e2e-framework branch 3 times, most recently from a75c224 to 25fb560 Compare July 13, 2022 17:52
@kkourt kkourt added the needs-rebase This PR needs to be rebased because it has merge conflicts. label Jul 14, 2022
@willfindlay willfindlay force-pushed the pr/willfindlay/e2e-framework branch from 25fb560 to d578a52 Compare July 14, 2022 15:57
@willfindlay willfindlay removed the needs-rebase This PR needs to be rebased because it has merge conflicts. label Jul 14, 2022
@kkourt kkourt added the area/testing Related to testing label Jul 15, 2022
Copy link
Contributor

@kkourt kkourt left a comment

Choose a reason for hiding this comment

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

Thanks William.

I have some minor questions/comments, but otherwise LGTM.

tests/e2e/checker/doc.go Show resolved Hide resolved
tests/e2e/checker/multiplexer.go Show resolved Hide resolved
tests/e2e/checker/rpcchecker.go Outdated Show resolved Hide resolved
func (rc *RPCChecker) CheckWithFilters(connTimeout time.Duration, allowList, denyList []*tetragon.Filter) features.Func {
return func(ctx context.Context, t *testing.T, _ *envconf.Config) context.Context {
rc.lock.Lock()
defer rc.lock.Unlock()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering why is this lock needed? AFAICT, it is only used in this function. Can this function be called concurrently from different goroutines? If so, this seems weird to me because I assumed we had one checker that talks to the multiplexer.

Copy link
Contributor Author

@willfindlay willfindlay Jul 19, 2022

Choose a reason for hiding this comment

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

If memory serves I was concerned about test authors using the same checker multiple times in a single testenv.TestInParallel. (Which runs its feature funcs concurrently.)

Copy link
Member

Choose a reason for hiding this comment

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

So maybe a comment here? to prevent accidental changes

Copy link
Contributor

Choose a reason for hiding this comment

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

I was concerned about test authors using the same checker multiple times in a single testenv.TestInParallel.

I see. Out of curiosity, how would one go about doing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kkourt testenv.TestInParallel(workloadFeature, checkerFeature, checkerFeature, checkerFeature /* etc... */)

addrs = append(addrs, fmt.Sprintf("localhost:%d", port))
}

if rc.getEvents == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be not nil? Can we enter the CheckWithFilters with .connect() already called? (AFAICT, this is the only place that getEvents is set)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the case where we want to use the same checker more than once we keep the original connection open. Useful for example when we want to run the same workload multiple times in a stress test.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the case where we want to use the same checker more than once we keep the original connection open. Useful for example when we want to run the same workload multiple times in a stress test.

Similar to my previous comment. How would one go about doing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kkourt just stick the workload + checker features in a for loop. For example you could imagine a TestKprobeNTimes test or something similar that runs the same test 1, 10, 20, etc. times in a row to check for flakes.

tests/e2e/helpers/dumpinfo.go Show resolved Hide resolved
@willfindlay willfindlay force-pushed the pr/willfindlay/e2e-framework branch from d578a52 to 2bd41df Compare July 19, 2022 14:02
@willfindlay
Copy link
Contributor Author

Looks like 4dac275 causes Tetragon to fail the skeleton test :(

@willfindlay willfindlay force-pushed the pr/willfindlay/e2e-framework branch 9 times, most recently from 8725bb1 to ec08611 Compare July 19, 2022 20:46
@willfindlay willfindlay added area/ci Related to CI area/e2e Related to the e2e-framework labels Jul 19, 2022
@willfindlay willfindlay force-pushed the pr/willfindlay/e2e-framework branch 3 times, most recently from f5db4e3 to 6d63a18 Compare July 25, 2022 20:45
@kkourt
Copy link
Contributor

kkourt commented Jul 26, 2022

@willfindlay since you seem to be still working on it, I'll move it to draft. Please mark it as ready when you feel it's ready to be merged.

@kkourt kkourt marked this pull request as draft July 26, 2022 08:17
@willfindlay
Copy link
Contributor Author

willfindlay commented Jul 26, 2022

@kkourt I think it's basically ready tbh (needs a squash/rebase obviously), but we should wait until the bug causing the new e2e job to fail has been fixed. @jrfastab says he is working on it atm. Let's keep it as draft until then 👍

@willfindlay willfindlay force-pushed the pr/willfindlay/e2e-framework branch from 6d63a18 to 02172ca Compare July 26, 2022 15:14
@willfindlay willfindlay changed the base branch from main to pr/willfindlay/fix-broken-cache July 26, 2022 20:12
@willfindlay willfindlay force-pushed the pr/willfindlay/e2e-framework branch from 02172ca to 94c965f Compare July 26, 2022 20:12
Base automatically changed from pr/willfindlay/fix-broken-cache to main July 27, 2022 01:08
@willfindlay willfindlay marked this pull request as ready for review July 27, 2022 01:09
@willfindlay
Copy link
Contributor Author

@kkourt I think it's in a good state to ship it now. I'll leave it for you just in case you want to do another round of comments in the morning.

This PR introduces a new end-to-end test framework in `tests/e2e` which we can use to
write cross-provider end-to-end tests for Tetragon. The tests themselves live in
`tests/e2e/tests`. I have added a skeleton file with lots of comments that folks can use
as a starting point for writing new tests.

Updated the Makefile to support e2e-tests by adding a separate e2e-test target and making
sure that regular `make test` will not call into the e2e-tests by pointing it directly at
`./pkg/...`. Also added an EXTRA_TESTFLAGS variable to the Makefile that can be used to
pass flags directly to the test binary from either a `make test` or `make e2e-test`.

Signed-off-by: William Findlay <[email protected]>
Add a basic job to run end-to-end tests in CI using a KinD cluster.

Signed-off-by: William Findlay <[email protected]>
@willfindlay willfindlay force-pushed the pr/willfindlay/e2e-framework branch from 94c965f to 158dd23 Compare July 27, 2022 20:53
@kkourt kkourt merged commit a9e4a9b into main Jul 28, 2022
@kkourt kkourt deleted the pr/willfindlay/e2e-framework branch July 28, 2022 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci Related to CI area/e2e Related to the e2e-framework area/testing Related to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants