-
Notifications
You must be signed in to change notification settings - Fork 574
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
a/snapasserts: keep track of components in validation sets #14370
base: master
Are you sure you want to change the base?
a/snapasserts: keep track of components in validation sets #14370
Conversation
cc8af3e
to
10457c3
Compare
10457c3
to
81baf20
Compare
4c38de1
to
0f7ce29
Compare
This is pretty big. I did some of the refactoring in a separate commit before introducing components, but I don't know if it makes much sense as a standalone PR. Let me know if this isn't easy to review, though. |
f7212b7
to
83df5ed
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.
thanks, just some minor comments
} | ||
} | ||
|
||
// TODO: maybe a method on snapContraints? |
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.
This seems appropriate in this case 👍
if ndiff > 1 { | ||
if cs.presence == asserts.PresenceRequired { | ||
|
||
cs.presence = derivePresence(cs.presence, comp.Presence, ndiff) |
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.
This last bit seems to not be covered by tests. Is this missing a test case?
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.
I messed up some of the tests during a rebase right before reviewing, this is fixed now and coverage is better. (I was accidentally excluding some of the cases in the table test I added).
There is a test missing for constraint conflicts that come from component constraints, adding that now.
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.
Added test here: ece0c19
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14370 +/- ##
==========================================
+ Coverage 78.85% 78.89% +0.03%
==========================================
Files 1079 1082 +3
Lines 145615 146186 +571
==========================================
+ Hits 114828 115332 +504
- Misses 23601 23650 +49
- Partials 7186 7204 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
70e831c
to
3b1ded9
Compare
…mponent constraints
Co-authored-by: Miguel Pires <miguelpires94@gmail.com>
Also, made the error strings for components contain the full identifier that includes both the snap and component names.
ece0c19
to
6b5bcf8
Compare
This will enable us to track components alongside snaps with validation sets.