RHOAIENG-17634: chore(odh-nbc/tests): implement diff minimization for k8s objects diffs #503
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…comparing CRs in kubeflow notebooks tests This is the initial refactor to use a new custom matcher. The matcher has no new features compared to the old way. I will do the meaningful-diffs part as a next PR (under the same Jira number).
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #503 +/- ##
===========================================
+ Coverage 55.27% 70.86% +15.58%
===========================================
Files 9 7 -2
Lines 2276 1335 -941
===========================================
- Hits 1258 946 -312
+ Misses 922 324 -598
+ Partials 96 65 -31 ☔ View full report in Codecov by Sentry. |
… k8s objects diffs This is a followup to the refactoring before. The minimal diffs are computed by taking the `actual` value, making a deep copy of it, applying the differences between `actual` and `expected` to the deep copy, and calling the comparison function repeatedly.
jiridanek
force-pushed
the
jd_followup_minimal_diff
branch
from
6c9317c to
a20d65e
Compare
jiridanek
added
ok-to-test
tide/merge-method-squash
and removed
do-not-merge/work-in-progress
labels
jstourac
reviewed
Jan 15, 2025
| if !vx.IsValid() || !vy.IsValid() { | ||
| // it may happen that more than one OnDiff will break off at the same place | ||
| // but that does not matter for correctness, it will just be a bit inefficient | ||
| break |
There was a problem hiding this comment.
Hmm, in this case - this diff isn't stored to the final diff - is it intentional? I'm not sure what are possible cases for these being invalid, so I'm not sure.
There was a problem hiding this comment.
it is, line 264, current.Set(vx), will run just like if we got at the end of the path.
There was a problem hiding this comment.
(one of) these will be invalid if the structs are not matched up, say both have a map, and one of them has a key and the other one does not; so the vy will be the value in that key and vx will be invalid because that key is missing and there's no value for that; there's a self test for this
// Structs contain different keys, so the comparison has to happen
// on a map as a whole, because the second struct lacks the key from first.
// This is something I forgot to handle originally.
func Test_BeMatchingK8sResource_MismatchedStructs(t *testing.T) {
someRoute := routev1.Route{
ObjectMeta: metav1.ObjectMeta{
Name: "someRouteName",
Labels: map[string]string{
"foo": "bar",
},
},
}
someOtherRoute := routev1.Route{
ObjectMeta: metav1.ObjectMeta{
Name: "someOtherRouteName",
Labels: nil,
},
}
...
|
Thank you @jiridanek for this. It is something that may be useful and can help during a dev or debugging. But I'm still a bit worried that this our implementation may sometimes mask some difference that actually matters. As such, what do you think to print both diffs? So, it would be first the standard diff as we have it now and then another diff that is minimized by this your implementation. WDYT? |
|
Here's what the diff looks like after the suggested change |
…mized diff this way we always have all the info when test fails, but it's still not difficult to find the minimized diff for initial diagnostics, because it's the most obvious thing at the bottom
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://issues.redhat.com/browse/RHOAIENG-17634
This is a followup on
Description
Implements the diff minimization when comparing K8s CRs. This is not trivial, because the comparison functions we use are general Go functions, and it's not easy to figure out why that function returns
falsefor a given input.Here's how it can be done. Create a deep copy of the
actualvalue, apply the differences betweenactualandexpectedone by one, and call the comparison function repeatedly to check whether each of the differences does or does not influence the comparison result.This uses the deep copy functions that K8s provides, and it also uses Go reflection to walk the compared structs, diff them, and modify them. Most of the diffing is done by the go-cmp package.
How Has This Been Tested?
Here's what the failure from the previous PR is looking now.
Very nice!
Merge criteria: