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

Test Command #236

Merged
merged 31 commits into from
Sep 26, 2024
Merged

Conversation

matthewnitschke-wk
Copy link
Contributor

Implements the proposal specified here: #235

Closes #235

docs/CLI.md Outdated Show resolved Hide resolved
Copy link
Contributor

@varungandhi-src varungandhi-src left a comment

Choose a reason for hiding this comment

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

Looks good in broad strokes, but I think there is a bunch of room to simplify things

cmd/scip/test.go Outdated Show resolved Hide resolved
cmd/scip/test.go Outdated Show resolved Hide resolved
cmd/scip/test.go Outdated Show resolved Hide resolved
cmd/scip/test.go Outdated Show resolved Hide resolved
docs/CLI.md Outdated Show resolved Hide resolved
cmd/scip/test.go Outdated
Comment on lines 106 to 107
// the type of attribute that this is
kind string
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make this an enum, not a string

cmd/scip/test.go Outdated Show resolved Hide resolved
cmd/scip/test.go Outdated Show resolved Hide resolved
cmd/scip/test.go Outdated Show resolved Hide resolved
cmd/scip/test_test.go Outdated Show resolved Hide resolved
@matthewnitschke-wk
Copy link
Contributor Author

@varungandhi-src amusingly long delay on this one, but finally got around to make your suggestions

Should be ready for another round of reviews!

Comment on lines +133 to +136
testCases := []TestCase{
{"roles",
autogold.Expect("✓ passes.repro (3 assertions)\n"),
autogold.Expect(`✗ fails-wrong-role.repro
Copy link
Contributor

Choose a reason for hiding this comment

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

@matthewnitschke-wk I've added snapshot tests here for checking the output, so that we don't accidentally regress something (earlier, this test was just checking for pass/fail instead of checking the output).

"github.com/stretchr/testify/require"
)

func TestTestCasesForLine(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@matthewnitschke-wk Thank you for adding this test, it compactly expresses the different kinds of syntax that are available and how they get parsed. Nice!

@varungandhi-src varungandhi-src merged commit 1ffdfa7 into sourcegraph:main Sep 26, 2024
3 checks passed
@varungandhi-src
Copy link
Contributor

@matthewnitschke-wk I've cut a new v0.5.0 release with the test subcommand.

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.

scip test command
2 participants