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

Test Local Driver #145

Merged
merged 12 commits into from
Oct 29, 2021
Merged

Conversation

willbeason
Copy link
Member

@willbeason willbeason commented Oct 11, 2021

The local driver has problems when trying to handle large numbers of templates.

Below is a performance test (included in this PR) which measures how long it takes to add N templates to the driver.

BenchmarkDriver_PutModule/1_templates-12         	    2696	    469,513 ns/op
BenchmarkDriver_PutModule/2_templates-12         	    1064	   1,124,514 ns/op
BenchmarkDriver_PutModule/5_templates-12         	     357	   3,530,500 ns/op
BenchmarkDriver_PutModule/10_templates-12        	     100	  10,047,225 ns/op
BenchmarkDriver_PutModule/20_templates-12        	      46	  31,473,562 ns/op
BenchmarkDriver_PutModule/50_templates-12        	       7	 159,611,378 ns/op
BenchmarkDriver_PutModule/100_templates-12       	       2	 720,247,533 ns/op

Note that performance is approximately n^2. This is due to the fact that:

  1. Compilation time for a rego runtime environment is proportional to the number of templates.
  2. We recompile for each template added.

A few PRs from now, I'm going to make a relatively complex change to try to resolve this issue. Our current tests are good at testing happy paths, but it's very possible I'll trigger changes to unhappy paths, and I want to be able to make those changes safely.

Thus, this PR adds a lot of unit tests for the local driver. There is overlap between local_test.go and local_unit_test.go but the two files are approaching the tests from different directions. The former mainly tests happy-path behaviors in sequences of calls, and local_unit_test.go strictly tests single operations, and as many paths as feasible to unit test.

To aid in testing and discovery I've moved local.Args to its own file, and defined a few new args to aid in writing tests. I've made a few small refactorings (e.g. extracting out the externalDataImpl function), but nothing that changes the overall flow of the code. Similarly, I've moved around how we instantiate the local.driver so testing is cleaner.

Other than that. I've avoided making logic changes except where it was very clear that the code was having an unintentional effect (such as continuing to process after an error state has been reached).

@willbeason willbeason self-assigned this Oct 11, 2021
@willbeason willbeason marked this pull request as draft October 11, 2021 17:11
@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2021

Codecov Report

Merging #145 (243b26b) into master (f478d8a) will increase coverage by 1.82%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #145      +/-   ##
==========================================
+ Coverage   42.15%   43.97%   +1.82%     
==========================================
  Files          55       56       +1     
  Lines        3117     3131      +14     
==========================================
+ Hits         1314     1377      +63     
+ Misses       1409     1379      -30     
+ Partials      394      375      -19     
Flag Coverage Δ
unittests 43.97% <ø> (+1.82%) ⬆️

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

Impacted Files Coverage Δ
...-agent/frameworks/constraint/pkg/client/backend.go 68.75% <0.00%> (ø)
...eworks/constraint/pkg/client/drivers/local/args.go 93.75% <0.00%> (ø)
...works/constraint/pkg/client/drivers/local/local.go 68.10% <0.00%> (+18.10%) ⬆️

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 f478d8a...243b26b. Read the comment docs.

@willbeason willbeason changed the title [WIP] Test local 2 Test Local Driver Oct 12, 2021
@willbeason willbeason marked this pull request as ready for review October 12, 2021 19:08
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

constraint/pkg/client/drivers/local/local_unit_test.go Outdated Show resolved Hide resolved
@maxsmythe
Copy link
Contributor

though it looks like there are failing tests

Will Beason added 11 commits October 27, 2021 11:35
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>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
@willbeason willbeason merged commit 8b4a99a into open-policy-agent:master Oct 29, 2021
@willbeason willbeason deleted the test-local-2 branch October 29, 2021 18:46
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

3 participants