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

simplify tests with cmp.Diff #475

Merged
merged 1 commit into from
Nov 15, 2024
Merged

Conversation

alexandear
Copy link
Contributor

This PR refactors tests. These code blocks

if !cmp.Equal(want, got) {
	t.Fatal(cmp.Diff(want, got))
}
if !bytes.Equal(want, got) {
	t.Fatal(cmp.Diff(want, got))
}
if want != got {
	t.Fatal(cmp.Diff(want, got))
}

changed to the following:

if diff := cmp.Diff(want, got); diff != "" {
	t.Fatal(diff)
}

@rhysd
Copy link
Owner

rhysd commented Nov 12, 2024

I intentionally avoid cmp.Diff because it requires extra performance cost to generate diff comparing to cmp.Equal.

@alexandear
Copy link
Contributor Author

I intentionally avoid cmp.Diff because it requires extra performance cost to generate diff comparing to cmp.Equal.

Could you provide more details on the performance difference between using cmp.Diff and cmp.Equal? Specifically, how much additional time does cmp.Diff take compared to cmp.Equal in tests or benchmarks?

Tests running time on my machine shows there is no significant difference.

Before:

time go test -shuffle=on -count=1 ./...
?       github.com/rhysd/actionlint/cmd/actionlint      [no test files]
ok      github.com/rhysd/actionlint     2.128s
ok      github.com/rhysd/actionlint/scripts/check-checks        1.216s
ok      github.com/rhysd/actionlint/scripts/generate-availability       1.350s
ok      github.com/rhysd/actionlint/scripts/generate-popular-actions    2.501s
ok      github.com/rhysd/actionlint/scripts/generate-webhook-events     1.776s
go test -shuffle=on -count=1 ./...  2.50s user 1.67s system 139% cpu 2.999 total

After:

time go test -shuffle=on -count=1 ./...
?       github.com/rhysd/actionlint/cmd/actionlint      [no test files]
ok      github.com/rhysd/actionlint     1.946s
ok      github.com/rhysd/actionlint/scripts/check-checks        1.183s
ok      github.com/rhysd/actionlint/scripts/generate-availability       0.814s
ok      github.com/rhysd/actionlint/scripts/generate-popular-actions    1.653s
ok      github.com/rhysd/actionlint/scripts/generate-webhook-events     1.673s
go test -shuffle=on -count=1 ./...  2.50s user 1.63s system 175% cpu 2.346 total

@rhysd rhysd force-pushed the main branch 2 times, most recently from 05e056b to 5aaa4ce Compare November 12, 2024 16:13
@rhysd
Copy link
Owner

rhysd commented Nov 15, 2024

Today I compared the performance and actually I couldn't find any performance differences between the APIs. I remember that there was some performance issue (due to a lot of allocations) in the past on my local but now it no longer exists.

@rhysd rhysd merged commit 9612d6a into rhysd:main Nov 15, 2024
14 checks passed
@rhysd
Copy link
Owner

rhysd commented Nov 15, 2024

Merged. Thanks!

@alexandear alexandear deleted the refactor-test-cmp-diff branch November 15, 2024 09:19
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.

2 participants