Conversation
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
release-note-none
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request streamlines the project's testing infrastructure by eliminating an external testing framework. By migrating existing tests to use standard Go testing practices with a popular assertion library, the change reduces the project's dependency footprint and aligns with common Go development patterns, potentially simplifying future maintenance and onboarding for new contributors. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughThis PR migrates test files from the gocheck framework ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@dveeden: GitHub didn't allow me to request PR reviews from the following users: dumanshu. Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/cc @wlwilliamx |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/diff/diff_test.go (1)
75-92: Minor: Consider consistent argument order forrequire.Equal.On line 86,
require.Equal(t, hash2, hash1)has arguments in a different order compared to line 91'srequire.NotEqual(t, hash1, hash3). For consistency and clearer error messages on failure, consider usingrequire.Equal(t, hash1, hash2)sincehash1is the expected reference value being compared against.This is a stylistic suggestion and doesn't affect test correctness.
Suggested fix
- require.Equal(t, hash2, hash1) + require.Equal(t, hash1, hash2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/diff/diff_test.go` around lines 75 - 92, In TestConfigHash, make the require.Equal assertion use the consistent expected-actual ordering by changing require.Equal(t, hash2, hash1) to require.Equal(t, hash1, hash2) so the expected reference value (hash1) is the first argument; locate this in the TestConfigHash function near the require calls comparing hash1, hash2, and hash3.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/diff/diff_test.go`:
- Around line 75-92: In TestConfigHash, make the require.Equal assertion use the
consistent expected-actual ordering by changing require.Equal(t, hash2, hash1)
to require.Equal(t, hash1, hash2) so the expected reference value (hash1) is the
first argument; locate this in the TestConfigHash function near the require
calls comparing hash1, hash2, and hash3.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
go.modpkg/diff/chunk_test.gopkg/diff/diff_test.gopkg/diff/merge_test.gopkg/diff/spliter_test.go
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to remove the pingcap/check dependency and replace its usage with stretchr/testify. The changes are generally well-executed. However, pingcap/check still remains as an indirect dependency in go.mod, which contradicts the main goal of this PR. Additionally, there are some minor opportunities for code simplification in the test files to make the assertions more idiomatic with testify. I've added specific comments with suggestions.
| github.com/philhofer/fwd v1.2.0 // indirect | ||
| github.com/pierrec/lz4 v2.6.1+incompatible // indirect | ||
| github.com/pingcap/badger v1.5.1-0.20241015064302-38533b6cbf8d // indirect | ||
| github.com/pingcap/check v0.0.0-20211026125417-57bd13f7b5f0 // indirect |
There was a problem hiding this comment.
The goal of this PR is to remove the pingcap/check dependency. However, it's still present as an indirect dependency. This might be because another dependency requires it, or go mod tidy hasn't been run after removing the direct usages.
Please run go mod tidy to ensure the dependency is fully removed. If it persists, use go mod why github.com/pingcap/check to identify which module is still using it and address that as well to fully complete the removal.
There was a problem hiding this comment.
$ go mod why github.com/pingcap/check
# github.com/pingcap/check
github.com/pingcap/ticdc/cmd/cdc/server
github.com/pingcap/tiflow/pkg/cmd/server
github.com/pingcap/tiflow/cdc/server
github.com/pingcap/tiflow/cdc/processor
github.com/pingcap/tiflow/pkg/sink/mysql
github.com/pingcap/tiflow/dm/pkg/conn
github.com/pingcap/check
Other dependencies still use this
ti-chi-bot
bot
added
do-not-merge/needs-triage-completed
and removed
do-not-merge/needs-linked-issue
labels
|
/check-issue-triage-complete |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wk989898 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
2 participants
What problem does this PR solve?
Issue Number: close #4301
This removes pingcap/check as direct dependency. Most of this repo already is using testify.
What is changed and how it works?
Check List
Tests
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note
Summary by CodeRabbit
Tests
Chores