-
Notifications
You must be signed in to change notification settings - Fork 49
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
Rewrite test TargetHandler #181
Conversation
Codecov Report
@@ Coverage Diff @@
## master #181 +/- ##
==========================================
- Coverage 45.70% 45.66% -0.05%
==========================================
Files 59 60 +1
Lines 2866 2847 -19
==========================================
- Hits 1310 1300 -10
+ Misses 1309 1302 -7
+ Partials 247 245 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
2f9a307
to
9e5361e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a few nits
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
This means the merge conflict won't cause compilation errors. Signed-off-by: Will Beason <willbeason@google.com>
Resolve various issues from now-merged commits, such as interface changes. Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Gatekeeper Test is failing |
Signed-off-by: Will Beason <willbeason@google.com>
Yes, this is expected for right now. @becky-hd is working on changing where/when we do validation. Right now Gatekeeper relies on CreateCRD to fail under certain conditions, when really we should have a dedicated ValidateCRD method. |
Since it is needed by several layers of abstraction, it should be in its own package. Ditto the test handler type. Thanks to @davis-haba for identifying this Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
The big idea here is that the current TargetHandler makes it hard to test several things we want to have good tests for before we can safely shard rego environments. At its core, this PR is about this new handler and the resulting test changes. This makes
pkg/client/clienttest/handler.go
a good starting point for reviewing this PR.I've broken "CRD Helpers" into individual methods and removed "CRDHelper" - this type was becoming a dumping ground of tenuously-related validation and constructing CRDs, ConstraintTemplates, and Constraints. Only one method used the single member of this type (the schema). So I've broken this into the
client/crds
package. The package could probably be better-named since not all of the functions in "CRDHelper" dealt with CRDs. Let me know if you have a better idea!Since the test Handler needs to import the
client
package and is in a test package, allclient
tests need to be in theclient_test
package. All functionality they rely on then needs to be exported byclient
. This change is responsible for much of the code changes.Lastly, the new test TargetHandler is much more flexible than the old one. This ended up making a lot of logic for existing E2E tests obsolete, so I've rewritten them. This makes the distinction between "e2e" and non-"e2e" tests a bit arbitrary as both flavors are using the same setup code (and have the same flexibility). I've arbitrarily decided to stick with the current convention that "e2e" tests are those which exercise "Review" or "Audit", or test sequences of behaviors (such as adding a Constraint, then deleting its Template).
Some changes in this PR are transitory - they are intended to be fixed in later PRs. These relate to making certain fields/methods exported which shouldn't be.
AllowedDataFields
,ParseModule
, andRequireModuleRules
are examples of these. They will be moved toclient/drivers/local
since that's the only location which should need that logic.