-
Notifications
You must be signed in to change notification settings - Fork 386
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
feat(test): allow gno test to default to the current directory #3453
base: master
Are you sure you want to change the base?
Conversation
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
remove: review/triage
flag
@@ -146,10 +146,6 @@ func (c *testCfg) RegisterFlags(fs *flag.FlagSet) { | |||
} | |||
|
|||
func execTest(cfg *testCfg, args []string, io commands.IO) error { | |||
if len(args) < 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you make it work by simply putting args
as []string{"."}
when it's empty? The current approach, verifying paths
, looks a bit hacky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting args
as []string{"."}
when it's empty works for simple cases like gno test
, but it falls apart once you add flags, because args
won't be empty anymore.
I also have to check if ./...
is in args
, since when you run a command with ./...
and no matching packages are found, the paths end up with a length of 0. That would incorrectly make it look like there are no path args, which is a false positive.
So while verifying paths might seem a bit clunky, it’s necessary to handle all these edge cases properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say this and the following test (valid_test
) are redundant, I would remove them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed those two tests.
This PR resolves #3420 by enhancing the gno test command.
The command now assumes the current directory (.) as the default path when no directory is specified, aligning its behavior with go test.
Changes to CI:
This PR is a repost of #3429 with a cleaner commit history.
CC @notJoon @moul