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

Add test codes for search_commands.go #3285

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tomfrombayesians
Copy link

Hello!

Since this is my first PR, I added a very small test. Our team is developing Tom, an open-source AI agent that automates code maintenance tasks like writing unit tests. This code was created through collaboration between Tom and me.

Right now, our project is still private, but if you’re interested, you can sign up for the Beta Release Waitlist through this link to get notified when it becomes available.

By the way, I ran into an issue when trying to execute the tests as described in the CONTRIBUTE guide. Even after running a Redis server locally and setting the required port, I couldn’t get the full test suite to run properly. To work around this, I created a TestSuite function inside search_commands_test.go to run the Ginkgo test in isolation.

Please let me know if anything needs to be adjusted! Thanks :)

@ofekshenawa
Copy link
Collaborator

Hey @tomfrombayesians , Thanks for your contribution!
Just a heads up, we’re about to update our testing infrastructure soon, which means we won’t need to run different servers or settings anymore. In addition, we will update the README to be more accurate.
Thanks again:)

@ndyakov
Copy link
Collaborator

ndyakov commented Feb 20, 2025

Hello @tomfrombayesians, there is a file search_test.go in which you can add your tests. We did merge couple of fixes related to the search that will be released soon (in 9.7.1). As for the testing infrastructure, you can look at #3274 , but the work on it is still in progress.

Comment on lines 3 to 11
import (
"reflect"

. "github.com/bsm/ginkgo/v2"
. "github.com/bsm/gomega"
"github.com/redis/go-redis/v9"
)

var _ = Describe("FTAggregateQuery", func() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should either be moved in search_test.go or move all the search_test.go logic here. Feel free to decide which file would you like to use.

Copy link
Author

Choose a reason for hiding this comment

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

I move all to search_test.go and also add some label.

Copy link
Collaborator

@ndyakov ndyakov left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. Left a suggestion in one of the tests.

Thank you for the contribution, @tomfrombayesians !

Comment on lines +866 to +871
for _, a := range args {
if reflect.DeepEqual(a, 500) {
found = true
break
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this can be rewriten as:

Suggested change
for _, a := range args {
if reflect.DeepEqual(a, 500) {
found = true
break
}
}
for i, a := range args {
if fmt.Sprintf("%s", a) == "TIMEOUT" {
Expect(fmt.Sprintf("%s", args[i+1])).To(Equal("500"))
found = true
break
}
}

And this way will check the order of arguments as well. Right now any arg can be 500 and this test will pass.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, and added bonus, we can drop reflect as direct dependancy.

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