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

Make Client E2E tests more flexible #150

Merged
merged 4 commits into from
Nov 30, 2021

Conversation

willbeason
Copy link
Member

@willbeason willbeason commented Nov 11, 2021

Refactor tests so that:

  • it is possible to specify rego and libraries per-test
  • all tests use their own Client
  • the Probe type is no longer necessary

This makes tests able to run hermetically. Additionally, In the future I'll be writing tests that use different Client configurations, which requires using different Clients for different tests (especially handler Rego).

Make response order more, but not completely, deterministic. We don't
need response order to be more deterministic for tests than what I've
put here, and as-is will make the output of reviewing objects more
consistent. There are common edge cases where ordering is still randomized,
but this can be fully fixed later.

Signed-off-by: Will Beason willbeason@google.com

@codecov-commenter
Copy link

codecov-commenter commented Nov 12, 2021

Codecov Report

Merging #150 (8e4b375) into master (217139c) will increase coverage by 0.65%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #150      +/-   ##
==========================================
+ Coverage   43.95%   44.60%   +0.65%     
==========================================
  Files          56       54       -2     
  Lines        3133     2717     -416     
==========================================
- Hits         1377     1212     -165     
+ Misses       1381     1259     -122     
+ Partials      375      246     -129     
Flag Coverage Δ
unittests 44.60% <ø> (+0.65%) ⬆️

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

Impacted Files Coverage Δ
...y-agent/frameworks/constraint/pkg/client/client.go 66.23% <0.00%> (-1.29%) ⬇️
...t/frameworks/constraint/pkg/client/probe_client.go
...gent/frameworks/constraint/pkg/client/e2e_tests.go

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 217139c...8e4b375. Read the comment docs.

@willbeason willbeason changed the title Make tests more flexible Make Client E2E tests more flexible Nov 12, 2021
@willbeason
Copy link
Member Author

GitHub isn't properly calculating the diffs. In all cases I've preserved the original test logic, and have only changed how the results are validated.

@maxsmythe
Copy link
Contributor

I wonder if there's some sort of non-determinism in the test coverage

Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

LGTM, 1 naming nit

constraint/pkg/client/e2e_test.go Show resolved Hide resolved
@willbeason
Copy link
Member Author

I wonder if there's some sort of non-determinism in the test coverage

Yeah, test coverage is nondeterministic

`
)

var denyAllCases = []struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not have positive cases as well? I can imagine a scenario where a bug is introduced (perhaps in the rego library) that causes all templates be rejected. These tests might still all/mostly pass.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. This PR is only for refactoring test cases so that adding such tests (which are indeed absolutely necessary) is easier. Having those in this PR would have made it too large.

Will Beason added 4 commits November 29, 2021 11:11
Signed-off-by: Will Beason <willbeason@google.com>
Refactor tests so that:

- it is possible to specify rego and libraries per-test
- all tests use their own Client
- the Probe type is no longer necessary

Signed-off-by: Will Beason <willbeason@google.com>
This makes test output on failure more consistent and clearer.

Use per-test Context instead of one shared between all tests. This
removes another avenue of test crosstalk and allows us to test things
like behavior with cancellable contexts.

Make response order more, but not completely, deterministic. We don't
need response order to be more deterministic for tests than what I've
put here, and as-is will make the output of reviewing objects more
consistent. There are common edge cases where ordering is randomized,
but this can be fully fixed later.

Comment out the contents of TestRemoteClientE2E. This test is currently
skipped until we have a working remote to connect the remote Driver to.
I've added clarifying comments to explain what we'll want once this
exists.

Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
c := &unstructured.Unstructured{}
c.SetGroupVersionKind(k8schema.GroupVersionKind{
Group: "constraints.gatekeeper.sh",
Version: "v1alpha1",
Copy link
Member

Choose a reason for hiding this comment

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

do we want to update apiVersions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. There's actually a few changes I want to make related to string constants, but I'd like to have a PR just for that:

#163

@willbeason willbeason merged commit 0087992 into open-policy-agent:master Nov 30, 2021
@willbeason willbeason deleted the add-tests branch November 30, 2021 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants