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

Add unit tests for regowriter #161

Merged
merged 4 commits into from
Jan 20, 2022

Conversation

becky-hd
Copy link
Contributor

Increase unit test coverage for constraint/pkg/regorewriter #157

@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2021

Codecov Report

Merging #161 (a205721) into master (4b0a3de) will increase coverage by 1.13%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #161      +/-   ##
==========================================
+ Coverage   45.20%   46.34%   +1.13%     
==========================================
  Files          59       59              
  Lines        2796     2792       -4     
==========================================
+ Hits         1264     1294      +30     
+ Misses       1290     1256      -34     
  Partials      242      242              
Flag Coverage Δ
unittests 46.34% <ø> (+1.13%) ⬆️

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

Impacted Files Coverage Δ
...t/frameworks/constraint/pkg/regorewriter/errors.go 26.66% <0.00%> (ø)
...eworks/constraint/pkg/regorewriter/regorewriter.go 74.68% <0.00%> (+20.36%) ⬆️

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 4b0a3de...a205721. Read the comment docs.

@maxsmythe
Copy link
Contributor

It looks like the lint test is failing? Also, feel free to assign Will as the reviewer once this PR is ready for a look.

constraint/pkg/regorewriter/regorewriter_test.go Outdated Show resolved Hide resolved
constraint/pkg/regorewriter/regorewriter_test.go Outdated Show resolved Hide resolved
constraint/pkg/regorewriter/regorewriter_test.go Outdated Show resolved Hide resolved
constraint/pkg/regorewriter/regorewriter_test.go Outdated Show resolved Hide resolved
constraint/pkg/regorewriter/regorewriter_test.go Outdated Show resolved Hide resolved
@becky-hd becky-hd force-pushed the regowriter-ut branch 8 times, most recently from 7eb63ca to 7fbe16d Compare December 7, 2021 04:37
constraint/pkg/regorewriter/regorewriter_test.go Outdated Show resolved Hide resolved
constraint/pkg/regorewriter/regorewriter_test.go Outdated Show resolved Hide resolved
constraint/pkg/regorewriter/regorewriter_test.go Outdated Show resolved Hide resolved
constraint/pkg/regorewriter/regorewriter_test.go Outdated Show resolved Hide resolved
constraint/pkg/regorewriter/regorewriter_test.go Outdated Show resolved Hide resolved
constraint/pkg/regorewriter/regorewriter_test.go Outdated Show resolved Hide resolved
constraint/pkg/regorewriter/regorewriter_test.go Outdated Show resolved Hide resolved
constraint/pkg/regorewriter/regorewriter_test.go Outdated Show resolved Hide resolved
constraint/pkg/regorewriter/regorewriter_test.go Outdated Show resolved Hide resolved
constraint/pkg/regorewriter/regorewriter_test.go Outdated Show resolved Hide resolved
constraint/pkg/regorewriter/regorewriter_test.go Outdated Show resolved Hide resolved
constraint/pkg/regorewriter/regorewriter_test.go Outdated Show resolved Hide resolved
constraint/pkg/regorewriter/regorewriter_test.go Outdated Show resolved Hide resolved
Copy link
Member

@willbeason willbeason left a comment

Choose a reason for hiding this comment

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

Looks great, thanks Becky!

@willbeason
Copy link
Member

@ritazh @shomron ?

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.

1 nit, one type conversion concern.

constraint/pkg/regorewriter/regorewriter.go Outdated Show resolved Hide resolved
constraint/pkg/regorewriter/regorewriter_test.go Outdated Show resolved Hide resolved
@becky-hd becky-hd force-pushed the regowriter-ut branch 2 times, most recently from 1a0663d to a205721 Compare January 13, 2022 03:48
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

Copy link
Member

@sozercan sozercan left a comment

Choose a reason for hiding this comment

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

minor nits, otherwise LGTM.

@becky-hd Looks like GK tests are failing, is this expected?

constraint/pkg/regorewriter/errors.go Outdated Show resolved Hide resolved
constraint/pkg/regorewriter/regorewriter_test.go Outdated Show resolved Hide resolved
Rewrite TestRegoRewriter to test AddLib and AddEntryPoint separately
Add go113 error for regowriter error assertion
Minor refactoring for rewriteImportPath

Signed-off-by: Becky Huang <beckyhd@google.com>
Signed-off-by: Becky Huang <beckyhd@google.com>
Signed-off-by: Becky Huang <beckyhd@google.com>
@willbeason
Copy link
Member

minor nits, otherwise LGTM.

@becky-hd Looks like GK tests are failing, is this expected?

Correct - this is currently a transient issue because we changed where validation is done. We're adding a dedicated "ValidateConstraintTemplate" method instead of having callers rely on implementation details of "CreateCRD".

Copy link
Member

@sozercan sozercan left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM

@willbeason willbeason merged commit 7950750 into open-policy-agent:master Jan 20, 2022
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