chore: improve code mark tests and remove worklog stubs#28
Conversation
- Expand italic and strikethrough tests to assert all inline nodes (not just code nodes), matching the thoroughness of the bold test - Expand the bold+codeBlock combo test with full node assertions - Remove unimplemented worklog/tempo command stubs and references from root command, README, and AGENTS.md
Summary of ChangesHello @Hinne1, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Merging this branch will not change overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the markdown conversion tests by adding detailed node-by-node assertions, and cleans up the codebase by removing unimplemented worklog command stubs. The documentation in README.md and AGENTS.md has been updated accordingly to reflect the removal of Tempo-related functionality. The changes are well-executed. I've left one suggestion to refactor the new test code to reduce duplication and improve maintainability by using a table-driven test structure.
| func TestMarkdownToADF_CodeInsideItalic(t *testing.T) { | ||
| adf := MarkdownToADF("*italic `code` here*") | ||
|
|
||
| if len(adf.Content) != 1 { | ||
| t.Fatalf("expected 1 block, got %d", len(adf.Content)) | ||
| } | ||
|
|
||
| para := adf.Content[0] | ||
| for _, c := range para.Content { | ||
| if hasCodeMark(c) { | ||
| if len(c.Marks) != 1 { | ||
| t.Errorf("code node should have only code mark, got %v", c.Marks) | ||
| } | ||
| } | ||
| if para.Type != "paragraph" { | ||
| t.Fatalf("expected paragraph, got %s", para.Type) | ||
| } | ||
|
|
||
| if len(para.Content) != 3 { | ||
| b, _ := json.MarshalIndent(adf, "", " ") | ||
| t.Fatalf("expected 3 inline nodes, got %d:\n%s", len(para.Content), b) | ||
| } | ||
|
|
||
| // "italic " should have em mark | ||
| if para.Content[0].Text != "italic " { | ||
| t.Errorf("expected 'italic ', got %q", para.Content[0].Text) | ||
| } | ||
| if len(para.Content[0].Marks) != 1 || para.Content[0].Marks[0].Type != "em" { | ||
| t.Errorf("expected [em] marks, got %v", para.Content[0].Marks) | ||
| } | ||
|
|
||
| // "code" should have ONLY code mark | ||
| if para.Content[1].Text != "code" { | ||
| t.Errorf("expected 'code', got %q", para.Content[1].Text) | ||
| } | ||
| if len(para.Content[1].Marks) != 1 || para.Content[1].Marks[0].Type != "code" { | ||
| t.Errorf("expected [code] marks only, got %v", para.Content[1].Marks) | ||
| } | ||
|
|
||
| // " here" should have em mark | ||
| if para.Content[2].Text != " here" { | ||
| t.Errorf("expected ' here', got %q", para.Content[2].Text) | ||
| } | ||
| if len(para.Content[2].Marks) != 1 || para.Content[2].Marks[0].Type != "em" { | ||
| t.Errorf("expected [em] marks, got %v", para.Content[2].Marks) | ||
| } | ||
| } | ||
|
|
||
| func TestMarkdownToADF_CodeInsideStrikethrough(t *testing.T) { | ||
| adf := MarkdownToADF("~~deleted `code` here~~") | ||
|
|
||
| if len(adf.Content) != 1 { | ||
| t.Fatalf("expected 1 block, got %d", len(adf.Content)) | ||
| } | ||
|
|
||
| para := adf.Content[0] | ||
| for _, c := range para.Content { | ||
| if hasCodeMark(c) { | ||
| if len(c.Marks) != 1 { | ||
| t.Errorf("code node should have only code mark, got %v", c.Marks) | ||
| } | ||
| } | ||
| if para.Type != "paragraph" { | ||
| t.Fatalf("expected paragraph, got %s", para.Type) | ||
| } | ||
|
|
||
| if len(para.Content) != 3 { | ||
| b, _ := json.MarshalIndent(adf, "", " ") | ||
| t.Fatalf("expected 3 inline nodes, got %d:\n%s", len(para.Content), b) | ||
| } | ||
|
|
||
| // "deleted " should have strike mark | ||
| if para.Content[0].Text != "deleted " { | ||
| t.Errorf("expected 'deleted ', got %q", para.Content[0].Text) | ||
| } | ||
| if len(para.Content[0].Marks) != 1 || para.Content[0].Marks[0].Type != "strike" { | ||
| t.Errorf("expected [strike] marks, got %v", para.Content[0].Marks) | ||
| } | ||
|
|
||
| // "code" should have ONLY code mark | ||
| if para.Content[1].Text != "code" { | ||
| t.Errorf("expected 'code', got %q", para.Content[1].Text) | ||
| } | ||
| if len(para.Content[1].Marks) != 1 || para.Content[1].Marks[0].Type != "code" { | ||
| t.Errorf("expected [code] marks only, got %v", para.Content[1].Marks) | ||
| } | ||
|
|
||
| // " here" should have strike mark | ||
| if para.Content[2].Text != " here" { | ||
| t.Errorf("expected ' here', got %q", para.Content[2].Text) | ||
| } | ||
| if len(para.Content[2].Marks) != 1 || para.Content[2].Marks[0].Type != "strike" { | ||
| t.Errorf("expected [strike] marks, got %v", para.Content[2].Marks) | ||
| } | ||
| } |
There was a problem hiding this comment.
Great job on making these tests more thorough! It's a solid improvement.
I noticed that there's a lot of repetitive boilerplate code across TestMarkdownToADF_CodeInsideItalic, TestMarkdownToADF_CodeInsideStrikethrough, and the existing TestMarkdownToADF_CodeInsideBold. The new assertions in TestMarkdownToADF_BoldWithCodeAndCodeBlock also follow this pattern.
To improve maintainability and make it easier to add new test cases, I suggest refactoring these into a single table-driven test. This would centralize the test logic and make the different cases clear and concise.
Here's an example of how you could structure it:
func TestMarkdownToADF_CodeInsideFormatting(t *testing.T) {
cases := []struct {
name string
markdown string
expectedNodes []struct {
text string
marks []string
}
}{
{
name: "code inside bold",
markdown: "**Bold `code` here**",
expectedNodes: []struct {
text string
marks []string
}{
{"Bold ", []string{"strong"}},
{"code", []string{"code"}},
{" here", []string{"strong"}},
},
},
{
name: "code inside italic",
markdown: "*italic `code` here*",
expectedNodes: []struct {
text string
marks []string
}{
{"italic ", []string{"em"}},
{"code", []string{"code"}},
{" here", []string{"em"}},
},
},
{
name: "code inside strikethrough",
markdown: "~~deleted `code` here~~",
expectedNodes: []struct {
text string
marks []string
}{
{"deleted ", []string{"strike"}},
{"code", []string{"code"}},
{" here", []string{"strike"}},
},
},
// You could also move the case from TestMarkdownToADF_BoldWithCodeAndCodeBlock here
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
adf := MarkdownToADF(tc.markdown)
if len(adf.Content) != 1 {
t.Fatalf("expected 1 block, got %d", len(adf.Content))
}
para := adf.Content[0]
if para.Type != "paragraph" {
t.Fatalf("expected paragraph, got %s", para.Type)
}
if len(para.Content) != len(tc.expectedNodes) {
b, _ := json.MarshalIndent(adf, "", " ")
t.Fatalf("expected %d inline nodes, got %d:\n%s", len(tc.expectedNodes), len(para.Content), b)
}
for i, expected := range tc.expectedNodes {
actual := para.Content[i]
if actual.Text != expected.text {
t.Errorf("node %d: expected text %q, got %q", i, expected.text, actual.Text)
}
if len(actual.Marks) != len(expected.marks) {
t.Errorf("node %d: expected %d marks, got %d (%v)", i, len(expected.marks), len(actual.Marks), actual.Marks)
continue
}
// A simple check for mark types. For more robustness, you could
// compare the slices of mark types irrespective of order.
for j, markType := range expected.marks {
if actual.Marks[j].Type != markType {
t.Errorf("node %d, mark %d: expected mark type %q, got %q", i, j, markType, actual.Marks[j].Type)
}
}
}
})
}
}1 participant
Summary
Test plan
golangci-lint runreports 0 issuesgo buildsucceeds without the worklog package