Skip to content

Conversation

@matthewnitschke-wk
Copy link
Contributor

Implements the proposal specified here: #235

Closes #235

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
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

@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