-
Notifications
You must be signed in to change notification settings - Fork 42
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 new checker ListEquals
#174
base: master
Are you sure you want to change the base?
Conversation
- implement slice diff algorithm
I wonder if the following format would be more readable, though it probably only works for reasonably short arrays:
|
@dimaqq : it's a nice idea. Unfortunately I don't think we have any control over the |
We already have the |
@manadart thanks, I'm aware of that one, this one is different though because it is order dependent. |
Add some table driven tests, benchmarks and fuzzing. - precheck for comparable types in ListEquals - correctly handle elems added/missing at start
e.g. a []string cannot equal an []any, even if they have the same contents - fix display of changed diffs - add some more test cases
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.
Seems like a cool idea. I would just ask that a best effort is made to explain the data structure more and what is going on. With fresh eyes this took me 20 minutes to understand and debug in my head.
} | ||
|
||
// "Traceback" to calculate the diff | ||
var diffs []diff |
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.
It seems far more simpler to just make diffs a []string
. We don't use the information in the diff
for anything else other then generating the string at printing time. Can we not just calculate it at the time we make the diff and store the result?
At the moment anything that implements the Stringer
interface could be a diff also.
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.
yeah, you're probably right
func generateDiff(obtained, expected reflect.Value) string { | ||
// lenLCS[m][n] stores the length of the longest common subsequence of | ||
// obtained[:m] and expected[:n] | ||
lenLCS := make([][]int, obtained.Len()+1) |
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 think we can clean up the memory allocations here. My 20 minute grok of this suggest we are blowing i.j.8 bytes for every check we perform when we can get away with (i+j).8 bytes.
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.
The full table is needed at the end to "trace backwards" and calculate the entire diff. Yes it's O(n*m) in space and time, the onus is on the user to not use this checker for gigantic arrays.
There might be slight optimisations to be made here but I don't think anything more efficient than O(n*m).
The current DeepEquals checker is not great at comparing lists and maps, and if they are not equal, DeepEquals generally fails to give a complete and concise description of the difference between them.
This is WIP to add a new checker which compares two lists for equality. In the case they are different, we essentially use a "diff" algorithm to find the difference between them. Thus we can give a much more useful description of the difference between two slices, e.g.
The canonical example of where this could be useful in Juju would be the supported series tests in older versions of Juju, when a new Ubuntu version is released.
Here's the output using
gc.DeepEquals
(spot the difference):gc.DeepEquals output
Here's the output using
jc.DeepEquals
(even worse):jc.DeepEquals output
And the output from
jc.ListEquals
:jc.ListEquals output