Skip to content

chore: Replace reflect.DeepEqual with cmp.Equal in tests#3691

Merged
gmlewis merged 3 commits into
google:masterfrom
jferrl:jferrl-reflect-refactor
Sep 22, 2025
Merged

chore: Replace reflect.DeepEqual with cmp.Equal in tests#3691
gmlewis merged 3 commits into
google:masterfrom
jferrl:jferrl-reflect-refactor

Conversation

@jferrl

@jferrl jferrl commented Aug 18, 2025

Copy link
Copy Markdown
Contributor

This PR refactors the test suite to replace all instances of reflect.DeepEqual with cmp.Equal from the github.com/google/go-cmp/cmp package. This change improves test performance, provides better error messages, and follows modern Go testing best practices. Also improves test output readability and consistency across the codebase

Updated multiple test files to use github.com/google/go-cmp/cmp.Equal instead of reflect.DeepEqual for value comparisons. This improves test output readability and consistency across the codebase.
@gmlewis

gmlewis commented Aug 18, 2025

Copy link
Copy Markdown
Collaborator

@jferri - just FYI - I lost write access to this repo and this may cause unexpected delays for which I apologize.
You can watch that other PR to check on the status.

@jferrl

jferrl commented Aug 18, 2025

Copy link
Copy Markdown
Contributor Author

@jferri - just FYI - I lost write access to this repo and this may cause unexpected delays for which I apologize. You can watch that other PR to check on the status.

No worries! 😃

@alexandear alexandear left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose configuring the forbidigo linter in the golangci-lint config to forbid the use of reflect.DeepEqual in tests. This way, the use of cmp.Equal will be automatically enforced in the future.

Configured the forbidigo linter in .golangci.yml to forbid the use of reflect.DeepEqual, recommending cmp.Equal instead. This helps ensure more reliable equality checks in the codebase.
@jferrl jferrl requested a review from alexandear August 19, 2025 07:27
@jferrl

jferrl commented Aug 19, 2025

Copy link
Copy Markdown
Contributor Author

I propose configuring the forbidigo linter in the golangci-lint config to forbid the use of reflect.DeepEqual in tests. This way, the use of cmp.Equal will be automatically enforced in the future.

Thanks for the suggestion, good catch 😃

@codecov

codecov Bot commented Sep 22, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.12%. Comparing base (3646f33) to head (a7aaaea).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3691   +/-   ##
=======================================
  Coverage   91.12%   91.12%           
=======================================
  Files         187      187           
  Lines       16640    16640           
=======================================
  Hits        15164    15164           
  Misses       1291     1291           
  Partials      185      185           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gmlewis gmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @jferrl and @alexandear!
LGTM.
Merging.

@gmlewis gmlewis merged commit 355aeb9 into google:master Sep 22, 2025
7 checks passed
jlaportebot added a commit to jlaportebot/go-github that referenced this pull request Jun 28, 2026
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.

3 participants