go-test / deep

Golang deep variable equality test that returns human-readable differences
MIT License
743 stars 54 forks source link

Add sorting of map keys and test tweaks #6

Closed aglyzov closed 4 years ago

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling b5fd28d9fd2401f9398ead0f61f6e066689ff09c on aglyzov:master into f49763a6ea0a91026be26f8213bebee726b4185f on go-test:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 6aa3df78ae8afe17f35b3a1794f3fcd382bdafe4 on aglyzov:master into f49763a6ea0a91026be26f8213bebee726b4185f on go-test:master.

daniel-nichter commented 7 years ago

Why sort map keys before comparing them? Also, why make the test part of the pkg?

aglyzov commented 7 years ago

1) well, sorting is needed if you display the diffs – you always get a consistent output which is (a) human friendly (easy to parse with eyes, i.e. you know where to look for "it") (b) machine friendly (and this was my use case actually – I have hundreds of items to compare in a regression test and I use the diff program to show changes from the last text snapshot)

2) simply because without it a fork of the project is untestable (the name changes and that breaks the import)

omeid commented 7 years ago

Please sort the diff result, not the map, please don't make deep mutate the test data.

aglyzov commented 7 years ago

@omeid, the patch does exactly this: sorts the diff results before they are passed to the caller. And of course nobody mutates the outside data.

aglyzov commented 7 years ago

Here, let me show an example, suppose we have the diffs logged like this:

FAIL: primary/avbt_ru-list.html     : FALSE POSITIVE [map[main:true prices:[[114200 RUB]] title:Kaiser EH 6000 Lift Glas] != <nil map>]
FAIL: primary/e96_ru-list.html      : FALSE POSITIVE [map[main:true prices:[[9190 RUB] [9010 RUB]] sku:89094 title:Мониторы в Екатеринбурге] != <nil map>]
FAIL: primary/eldorado_ru-list.html : FALSE POSITIVE [map[main:true prices:[[30029 RUB]] title:Моющие пылесосы] != <nil map>]

And suppose also we have hundreds of them. So in order to have some control on what's going on we save the test output to a file and then use the diff program to check what has changed. Like this: make test-regressions | diff -u regression-tests-snapshot.txt -

But, there is a catch – due to the non-deterministic nature of hash-maps their keys come out in unpredictable order. I.e. next time it could be FALSE POSITIVE [map[title:* main:* prices:*] and the diff program will report the change where there is none.

mibk commented 6 years ago

I agree that the output should be deterministic. That's always better. Anyway, I wouldn't make deep_test.go a part of the package. The argument

Removing unnecessary reference to “github.com/go-test/deep” helps testing github forks. It is safe to have the testing a part of the package because *_test.go files only get compiled when go test is invoked.

isn't solid enough. See http://blog.campoy.cat/2014/03/github-and-go-forking-pull-requests-and.html for handling GitHub forks.

Then, the 1st and the 3rd commit should be squashed.

CpuID commented 5 years ago

Any movement on this? +100 for sorted deterministic diffs, to allow machine parsing them to validate expectations/spot regressions.

Anaminus commented 5 years ago

As of Go 1.12, fmt prints maps in key-sorted order.

daniel-nichter commented 4 years ago

Agreed that deterministic results/output are ideal, especially for testing. Since the Go change @Anaminus mentioned, I think this feature needs to be reimplemented in a new PR. (Also, as @mibk said, deep_test.go does not need to be part of the pkg.)

Will close this PR for now, but again: I support the idea. Just need a cleaner, go1.12-aware implementation of it.