e2e: Run tests as sub-tests #242

JAORMX · 2021-01-28T07:38:54Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Currently, the tests e2e are being from a single TestSuite which
iterates through the test definitions calling their respective
functions.

However, if one of these definitions were to fail, the error is reported
towards the upper TestSuite test.

This uses testify's Suite object ability to run sub-tests (similar to
what testing.T provides) and instead uses that.

This way, any errors coming from a sub-test will be reported for that
sub-test and it'll be easier to debug.

Does this PR have test?

N/A

Special notes for your reviewer:

One thing to note is that given our usage of testify's Suite, we
aren't able to do parallel testing in our e2e tests. In order to enable
this, we'd need to move away from the Suite object or use a different
framework.

See stretchr/testify#187

Does this PR introduce a user-facing change?

NONE

codecov-io · 2021-01-28T07:41:32Z

Codecov Report

Merging #242 (3d41bb7) into master (6de296f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #242   +/-   ##
=======================================
  Coverage   30.25%   30.25%           
=======================================
  Files           5        5           
  Lines         476      476           
=======================================
  Hits          144      144           
  Misses        315      315           
  Partials       17       17

Flag	Coverage Δ
unittests	`30.25% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6de296f...3d41bb7. Read the comment docs.

JAORMX · 2021-01-28T09:16:31Z

/retest

JAORMX · 2021-01-28T09:43:06Z

/retest

saschagrunert · 2021-01-28T12:06:31Z

test/suite_test.go

+// We're unable to use parallel tests because of our usage of testify/suite.
+// See https://github.com/stretchr/testify/issues/187
+//
+// nolint:paralleltest


I'm not sure we would want parallel tests anyway, since these are integration tests making changes to a live system, if they were run in parallel they might interfere with each other.

naveensrinivasan · 2021-01-28T15:35:37Z

@JAORMX: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-security-profiles-operator-test-e2e 4255c25 link /test pull-security-profiles-operator-test-e2e
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Running into the same error Expected nil, but got: command /usr/local/bin/kubectl -n security-profiles-operator wait --for condition=ready pod -l app=spod did not succeed: error: no matching resources found

JAORMX · 2021-01-30T14:18:16Z

/retest

pjbgf

Nice one @JAORMX! This will make it easier for contributors to understand the source of errors. 👍

The issue about being unable to run parallel tests we can resolve on another PR. Holding for any extra thoughts from @saschagrunert

/lgtm
/hold

pjbgf · 2021-02-01T21:48:06Z

Assuming the issue on the E2E tests was resolved by #252

/test pull-security-profiles-operator-test-e2e

saschagrunert · 2021-02-02T08:24:44Z

Looks like the test failure is not transient:
/retest

JAORMX · 2021-02-03T08:20:43Z

I removed the per-section "Run" statements, and limited this PR to only the actual sub-tests. Let's see if this works better in CI.

saschagrunert · 2021-02-03T08:35:11Z

/retest

Currently, the tests e2e are being from a single `TestSuite` which iterates through the test definitions calling their respective functions. However, if one of these definitions were to fail, the error is reported towards the upper `TestSuite` test. This uses testify's `Suite` object ability to run sub-tests (similar to what `testing.T` provides) and instead uses that. This way, any errors coming from a sub-test will be reported for that sub-test and it'll be easier to debug. One thing to note is that given our usage of testify's `Suite`, we aren't able to do parallel testing in our e2e tests. In order to enable this, we'd need to move away from the `Suite` object or use a different framework. Signed-off-by: Juan Antonio Osorio Robles <jaosorior@redhat.com>

JAORMX · 2021-02-03T12:04:40Z

/test pull-security-profiles-operator-test-e2e

We don't support parallelization there yet anyway. Signed-off-by: Juan Antonio Osorio Robles <jaosorior@redhat.com>

It was not working in the namespace tests. Signed-off-by: Juan Antonio Osorio Robles <jaosorior@redhat.com>

saschagrunert

/lgtm

k8s-ci-robot · 2021-02-03T12:36:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JAORMX, pjbgf, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [pjbgf,saschagrunert]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

saschagrunert · 2021-02-03T12:36:53Z

/hold cancel

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 28, 2021

k8s-ci-robot requested review from cmurphy and pjbgf January 28, 2021 07:39

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 28, 2021

JAORMX force-pushed the sub-tests branch 3 times, most recently from cd64c5b to 6bb856f Compare January 28, 2021 08:09

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 28, 2021

saschagrunert reviewed Jan 28, 2021

View reviewed changes

JAORMX force-pushed the sub-tests branch 2 times, most recently from 9ccfeb8 to e593b14 Compare January 29, 2021 14:04

pjbgf approved these changes Feb 1, 2021

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 1, 2021

k8s-ci-robot assigned pjbgf Feb 1, 2021

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 1, 2021

JAORMX force-pushed the sub-tests branch from e593b14 to 78b81aa Compare February 2, 2021 11:27

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 2, 2021

JAORMX force-pushed the sub-tests branch 2 times, most recently from 57f1af7 to d5cdb72 Compare February 3, 2021 08:20

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 3, 2021

JAORMX force-pushed the sub-tests branch from 856bc7f to 3d41bb7 Compare February 3, 2021 11:48

JAORMX added 2 commits February 3, 2021 14:18

Explicitly disable parallelization in e2e test call

cacf557

We don't support parallelization there yet anyway. Signed-off-by: Juan Antonio Osorio Robles <jaosorior@redhat.com>

Only run ProfileBinding e2e test on cluster-wide tests

ab0c4bc

It was not working in the namespace tests. Signed-off-by: Juan Antonio Osorio Robles <jaosorior@redhat.com>

JAORMX force-pushed the sub-tests branch from 3d41bb7 to ab0c4bc Compare February 3, 2021 12:23

JAORMX requested a review from saschagrunert February 3, 2021 12:36

saschagrunert approved these changes Feb 3, 2021

View reviewed changes

k8s-ci-robot assigned saschagrunert Feb 3, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 3, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 3, 2021

k8s-ci-robot merged commit 5680077 into kubernetes-sigs:master Feb 3, 2021

JAORMX deleted the sub-tests branch February 3, 2021 12:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2e: Run tests as sub-tests #242

e2e: Run tests as sub-tests #242

JAORMX commented Jan 28, 2021 •

edited

Loading

codecov-io commented Jan 28, 2021 •

edited

Loading

JAORMX commented Jan 28, 2021

JAORMX commented Jan 28, 2021

saschagrunert Jan 28, 2021

cmurphy Feb 2, 2021

naveensrinivasan commented Jan 28, 2021

JAORMX commented Jan 30, 2021

pjbgf left a comment

pjbgf commented Feb 1, 2021 •

edited

Loading

saschagrunert commented Feb 2, 2021

JAORMX commented Feb 3, 2021

saschagrunert commented Feb 3, 2021

JAORMX commented Feb 3, 2021

saschagrunert left a comment

k8s-ci-robot commented Feb 3, 2021

saschagrunert commented Feb 3, 2021

e2e: Run tests as sub-tests #242

e2e: Run tests as sub-tests #242

Conversation

JAORMX commented Jan 28, 2021 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Does this PR have test?

Special notes for your reviewer:

Does this PR introduce a user-facing change?

codecov-io commented Jan 28, 2021 • edited Loading

Codecov Report

JAORMX commented Jan 28, 2021

JAORMX commented Jan 28, 2021

saschagrunert Jan 28, 2021

Choose a reason for hiding this comment

cmurphy Feb 2, 2021

Choose a reason for hiding this comment

naveensrinivasan commented Jan 28, 2021

JAORMX commented Jan 30, 2021

pjbgf left a comment

Choose a reason for hiding this comment

pjbgf commented Feb 1, 2021 • edited Loading

saschagrunert commented Feb 2, 2021

JAORMX commented Feb 3, 2021

saschagrunert commented Feb 3, 2021

JAORMX commented Feb 3, 2021

saschagrunert left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 3, 2021

saschagrunert commented Feb 3, 2021

JAORMX commented Jan 28, 2021 •

edited

Loading

codecov-io commented Jan 28, 2021 •

edited

Loading

pjbgf commented Feb 1, 2021 •

edited

Loading