Skip to content

Commit 7db6940

Browse files
committed
Remove todo metadata key and related code
- Remove Todo field from testMetadata struct in parser_test.go - Remove -check-skipped flag and related logic - Remove todo key from 146 metadata.json files - Update cmd/next-test to only support -format flag - Update CLAUDE.md documentation The explain_todo key now provides more granular control for skipping specific statement tests within a test case.
1 parent 03fef2b commit 7db6940

File tree

149 files changed

+457
-244
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

149 files changed

+457
-244
lines changed

CLAUDE.md

Lines changed: 3 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,6 @@
22

33
## Next Steps
44

5-
To find the next test to work on, run:
6-
7-
```bash
8-
go run ./cmd/next-test
9-
```
10-
11-
This tool finds all tests with `todo: true` in their metadata and returns the one with the shortest `query.sql` file.
12-
135
To find the next format roundtrip test to work on, run:
146

157
```bash
@@ -18,18 +10,6 @@ go run ./cmd/next-test -format
1810

1911
This finds tests with `todo_format: true` in their metadata.
2012

21-
## Workflow
22-
23-
1. Run `go run ./cmd/next-test` to find the next test to implement
24-
2. Check the test's `query.sql` to understand what ClickHouse SQL needs parsing
25-
3. Check the test's `explain.txt` to understand the expected EXPLAIN output
26-
4. Implement the necessary AST types in `ast/`
27-
5. Add parser logic in `parser/parser.go`
28-
6. Update the `Explain()` function if needed to match ClickHouse's output format
29-
7. Enable the test by removing `todo: true` from its `metadata.json` (set it to `{}`)
30-
8. Run `go test ./parser/... -timeout 5s` to verify
31-
9. Check if other todo tests now pass (see below)
32-
3313
## Running Tests
3414

3515
Always run parser tests with a 5 second timeout:
@@ -40,16 +20,6 @@ go test ./parser/... -timeout 5s
4020

4121
The tests are very fast. If a test is timing out, it indicates a bug (likely an infinite loop in the parser).
4222

43-
## Checking for Newly Passing Todo Tests
44-
45-
After implementing parser changes, run:
46-
47-
```bash
48-
go test ./parser/... -check-skipped -v 2>&1 | grep "PASSES NOW"
49-
```
50-
51-
Tests that output `PASSES NOW` can have their `todo` flag removed from `metadata.json`. This helps identify when parser improvements fix multiple tests at once.
52-
5323
## Checking for Newly Passing Format Tests
5424

5525
After implementing format changes, run:
@@ -64,14 +34,15 @@ Tests that output `FORMAT PASSES NOW` can have their `todo_format` flag removed
6434

6535
Each test in `parser/testdata/` contains:
6636

67-
- `metadata.json` - `{}` for enabled tests, `{"todo": true}` for pending tests
37+
- `metadata.json` - `{}` for enabled tests
6838
- `query.sql` - ClickHouse SQL to parse
6939
- `explain.txt` - Expected EXPLAIN AST output (matches ClickHouse's format)
40+
- `explain_N.txt` - Expected EXPLAIN AST output for Nth statement (N >= 2)
7041

7142
### Metadata Options
7243

73-
- `todo: true` - Test is pending parser/explain implementation
7444
- `todo_format: true` - Format roundtrip test is pending implementation
45+
- `explain_todo: {"stmt2": true}` - Skip specific statement subtests
7546
- `skip: true` - Skip test entirely (e.g., causes infinite loop)
7647
- `explain: false` - Skip test (e.g., ClickHouse couldn't parse it)
7748
- `parse_error: true` - Query is intentionally invalid SQL

cmd/next-test/main.go

Lines changed: 13 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,9 @@ import (
99
"sort"
1010
)
1111

12-
var formatFlag = flag.Bool("format", false, "Find tests with todo_format instead of todo")
12+
var formatFlag = flag.Bool("format", false, "Find tests with todo_format (required)")
1313

1414
type testMetadata struct {
15-
Todo bool `json:"todo,omitempty"`
1615
TodoFormat bool `json:"todo_format,omitempty"`
1716
Explain *bool `json:"explain,omitempty"`
1817
Skip bool `json:"skip,omitempty"`
@@ -27,6 +26,12 @@ type todoTest struct {
2726
func main() {
2827
flag.Parse()
2928

29+
if !*formatFlag {
30+
fmt.Fprintf(os.Stderr, "Usage: go run ./cmd/next-test -format\n")
31+
fmt.Fprintf(os.Stderr, "Finds tests with todo_format: true in metadata.\n")
32+
os.Exit(1)
33+
}
34+
3035
testdataDir := "parser/testdata"
3136
entries, err := os.ReadDir(testdataDir)
3237
if err != nil {
@@ -55,15 +60,9 @@ func main() {
5560
continue
5661
}
5762

58-
// Check for todo or todo_format based on flag
59-
if *formatFlag {
60-
if !metadata.TodoFormat {
61-
continue
62-
}
63-
} else {
64-
if !metadata.Todo {
65-
continue
66-
}
63+
// Check for todo_format
64+
if !metadata.TodoFormat {
65+
continue
6766
}
6867

6968
// Skip tests with skip or explain=false or parse_error
@@ -84,13 +83,8 @@ func main() {
8483
})
8584
}
8685

87-
todoType := "todo"
88-
if *formatFlag {
89-
todoType = "todo_format"
90-
}
91-
9286
if len(todoTests) == 0 {
93-
fmt.Printf("No %s tests found!\n", todoType)
87+
fmt.Printf("No todo_format tests found!\n")
9488
return
9589
}
9690

@@ -103,7 +97,7 @@ func main() {
10397
next := todoTests[0]
10498
testDir := filepath.Join(testdataDir, next.name)
10599

106-
fmt.Printf("Next %s test: %s\n\n", todoType, next.name)
100+
fmt.Printf("Next todo_format test: %s\n\n", next.name)
107101

108102
// Print query.sql contents
109103
queryPath := filepath.Join(testDir, "query.sql")
@@ -116,5 +110,5 @@ func main() {
116110
fmt.Printf("\nExpected EXPLAIN output:\n%s\n", string(explainBytes))
117111
}
118112

119-
fmt.Printf("\nRemaining %s tests: %d\n", todoType, len(todoTests))
113+
fmt.Printf("\nRemaining todo_format tests: %d\n", len(todoTests))
120114
}

parser/parser_test.go

Lines changed: 2 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -15,17 +15,12 @@ import (
1515
"github.com/sqlc-dev/doubleclick/parser"
1616
)
1717

18-
// checkSkipped runs skipped todo tests to see which ones now pass.
19-
// Use with: go test ./parser -check-skipped -v
20-
var checkSkipped = flag.Bool("check-skipped", false, "Run skipped todo tests to see which ones now pass")
21-
2218
// checkFormat runs skipped todo_format tests to see which ones now pass.
2319
// Use with: go test ./parser -check-format -v
2420
var checkFormat = flag.Bool("check-format", false, "Run skipped todo_format tests to see which ones now pass")
2521

2622
// testMetadata holds optional metadata for a test case
2723
type testMetadata struct {
28-
Todo bool `json:"todo,omitempty"`
2924
TodoFormat bool `json:"todo_format,omitempty"` // true if format roundtrip test is pending
3025
ExplainTodo map[string]bool `json:"explain_todo,omitempty"` // map of stmtN -> true to skip specific statements
3126
Source string `json:"source,omitempty"`
@@ -116,7 +111,6 @@ func findCommentStart(line string) int {
116111
// Each subdirectory in testdata represents a test case with:
117112
// - query.sql: The SQL query to parse (may contain multiple statements)
118113
// - metadata.json (optional): Metadata including:
119-
// - todo: true if the test is not yet expected to pass
120114
// - explain: false to skip the test (e.g., when ClickHouse couldn't parse it)
121115
// - skip: true to skip the test entirely (e.g., causes infinite loop)
122116
// - parse_error: true if the query is intentionally invalid SQL (expected to fail parsing)
@@ -164,12 +158,9 @@ func TestParser(t *testing.T) {
164158
}
165159

166160
// Skip tests where explain is explicitly false (e.g., ClickHouse couldn't parse it)
167-
// Unless -check-skipped is set and the test is a todo test
168161
if metadata.Explain != nil && !*metadata.Explain {
169-
if !(*checkSkipped && metadata.Todo) {
170-
t.Skipf("Skipping: explain is false in metadata")
171-
return
172-
}
162+
t.Skipf("Skipping: explain is false in metadata")
163+
return
173164
}
174165

175166
// Split into individual statements
@@ -219,14 +210,6 @@ func TestParser(t *testing.T) {
219210
t.Skipf("Expected parse error (intentionally invalid SQL)")
220211
return
221212
}
222-
if metadata.Todo {
223-
if *checkSkipped {
224-
t.Skipf("STILL FAILING (parse error): %v", parseErr)
225-
} else {
226-
t.Skipf("TODO: Parser does not yet support (error: %v)", parseErr)
227-
}
228-
return
229-
}
230213
t.Fatalf("Parse error: %v", parseErr)
231214
}
232215

@@ -239,14 +222,6 @@ func TestParser(t *testing.T) {
239222
// Verify we can serialize to JSON
240223
_, jsonErr := json.Marshal(stmts[0])
241224
if jsonErr != nil {
242-
if metadata.Todo {
243-
if *checkSkipped {
244-
t.Skipf("STILL FAILING (JSON serialization): %v", jsonErr)
245-
} else {
246-
t.Skipf("TODO: JSON serialization failed: %v", jsonErr)
247-
}
248-
return
249-
}
250225
t.Fatalf("JSON marshal error: %v\nQuery: %s", jsonErr, stmt)
251226
}
252227

@@ -260,14 +235,6 @@ func TestParser(t *testing.T) {
260235
actual := strings.TrimSpace(parser.Explain(stmts[0]))
261236
// Use case-insensitive comparison since ClickHouse EXPLAIN AST has inconsistent casing
262237
if !strings.EqualFold(actual, expected) {
263-
if metadata.Todo {
264-
if *checkSkipped {
265-
t.Skipf("STILL FAILING (explain mismatch):\nExpected:\n%s\n\nGot:\n%s", expected, actual)
266-
} else {
267-
t.Skipf("TODO: Explain output mismatch\nQuery: %s\nExpected:\n%s\n\nGot:\n%s", stmt, expected, actual)
268-
}
269-
return
270-
}
271238
t.Errorf("Explain output mismatch\nQuery: %s\nExpected:\n%s\n\nGot:\n%s", stmt, expected, actual)
272239
}
273240
}
@@ -302,18 +269,6 @@ func TestParser(t *testing.T) {
302269
}
303270
}
304271

305-
// If we get here with a todo test and -check-skipped is set on first statement, the test passes!
306-
if stmtIndex == 1 && metadata.Todo && *checkSkipped {
307-
metadata.Todo = false
308-
updatedBytes, err := json.Marshal(metadata)
309-
if err != nil {
310-
t.Errorf("Failed to marshal updated metadata: %v", err)
311-
} else if err := os.WriteFile(metadataPath, append(updatedBytes, '\n'), 0644); err != nil {
312-
t.Errorf("Failed to write updated metadata.json: %v", err)
313-
} else {
314-
t.Logf("ENABLED - removed todo flag from: %s", entry.Name())
315-
}
316-
}
317272
})
318273
}
319274
})
Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,3 @@
1-
{"todo":true,"todo_format":true}
1+
{
2+
"todo_format": true
3+
}
Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,3 @@
1-
{"todo":true,"todo_format":true}
1+
{
2+
"todo_format": true
3+
}
Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,3 @@
1-
{"todo":true,"todo_format":true}
1+
{
2+
"todo_format": true
3+
}
Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,3 @@
1-
{"todo":true,"todo_format":true}
1+
{
2+
"todo_format": true
3+
}
Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,3 @@
1-
{"todo":true,"todo_format":true}
1+
{
2+
"todo_format": true
3+
}
Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,3 @@
1-
{"todo":true,"todo_format":true}
1+
{
2+
"todo_format": true
3+
}
Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,3 @@
1-
{"todo":true,"todo_format":true}
1+
{
2+
"todo_format": true
3+
}

0 commit comments

Comments
 (0)