Repository navigation
internal/graph: escape source filenames in DOT labels - #1032
jabrailkhalil wants to merge 13 commits into
Conversation
| compareGraphs(t, buf.Bytes(), "compose7.dot") | ||
| } | ||
|
|
||
| func TestComposeWithFilenamesThatNeedEscaping(t *testing.T) { |
There was a problem hiding this comment.
Can we just add these test cases to the TestComposeWithNamesThatNeedEscaping test above?
There was a problem hiding this comment.
Consolidated the filename cases into TestComposeWithNamesThatNeedEscaping and removed the separate multilinePrintableName test. The compose golden cases cover quotes, newlines and literal backslashes, including line/column labels; the backslash case is skipped on Windows because it is a path separator there. Updated in 0424813. The root Go suite, graph race test and graph vet pass.
| } | ||
| } | ||
|
|
||
| func TestMultilinePrintableNameWithFilenamesThatNeedEscaping(t *testing.T) { |
There was a problem hiding this comment.
Does this test buy us any coverage over the compose test above? Maybe just add the relevant test cases there?
There was a problem hiding this comment.
Removed the redundant direct-helper test; its filename cases now run through the compose/golden test, as requested. The production fix is unchanged.
Signed-off-by: jabrailkhalil <jabrailkhalil@gmail.com>
|
I consolidated the filename regressions into the existing compose-test table as requested and updated this branch from main. On current head |
| name: "filenames with quotes and newlines", | ||
| src: NodeInfo{Name: "src", File: `src/file"name.cc`, Lineno: 10, Columnno: 2}, | ||
| dest: NodeInfo{File: "src/file\nname.cc"}, | ||
| want: "compose_filenames.dot", |
There was a problem hiding this comment.
Can we use one file per test function like before? When I mentioned consolidating I didn't mean just handling two files in the same funciton.
There was a problem hiding this comment.
I restored one golden file per test function. Quote/newline filenames now extend the existing escaping-name graph and share compose7.dot; I removed the redundant filename golden. Literal backslashes have one separate compose test because Windows treats them as path separators. The production fix is unchanged. Linux graph tests, race tests, vet, gofmt, and Graphviz parsing pass; both regressions fail if filename escaping is removed. The quote/newline test also passes when the cross-compiled test binary is run on Windows, with the backslash test skipped as intended.
|
Integrated current main The graph package tests and scoped go vet pass. The backslash-filename case retains its existing Windows skip; it was not executed as a Linux filesystem test. Fresh validation uses Windows Go 1.26.0, inherited from current main. The full Windows suite is not green: the three PE/DWARF mapping subcases also fail on clean current main in the same environment. The initial run additionally lacked the external diff executable; it is not treated as a source regression. Browser modules retain their x86-64 Linux guards and skip execution on Windows: compilation is not a new Linux/browser/race validation. No existing assertions, timing thresholds, platform guards or dependency versions were weakened. Current-head upstream CI and maintainer review are still needed. Could a maintainer approve the new-head workflow and review the integration? |
|
Validated unchanged head Both filename-escaping regressions actually execute and pass on Linux, including the literal-backslash case. With only the regression test/golden changes applied to main, both fail with the expected unescaped DOT-label differences; no production file was changed in the control. Main passed the same Linux commands. Module manifests, checksums and Update October 12: GitHub now reports this historical matrix job as completed/SUCCESS, consistent with its raw evidence. Current-head validation is linked in the updated PR description. Could a maintainer authorize the upstream workflow and review the current-head integration? |
| compareGraphs(t, buf.Bytes(), "compose7.dot") | ||
| } | ||
|
|
||
| func TestComposeWithFilenamesWithBackslashes(t *testing.T) { |
There was a problem hiding this comment.
Can this case be a part of TestComposeWithNamesThatNeedEscaping?
There was a problem hiding this comment.
Done in cb195a7: the backslash-label assertion is now part of TestComposeWithNamesThatNeedEscaping, and the separate test/golden is removed. Quote/newline coverage still runs on Windows. Linux passes; both the golden check and backslash assertion fail against unchanged main production code.
|
@jabrailkhalil Having reviewed a few of your PRs, they require quite a few iterations and have substantial amount of AI generated traffic that appears to be unreviewed / unshaped by the author. Can I ask you to either curate the changes more to make them more concise and suitable in their size for their value, or to stop contributing? Related: Are you an active pprof user? I updated CONTRIBUTING.md a little to clarify the guidelines. In particular, we expect most contributions to be from active pprof users contributing fixes for the problems they encounter in their work. We are less interested in contributions that just go over the backlog to fix things: Some of those are lower priority and they are not worth working on (either for authors or reviewers) before there is enough user feedback to prioritize them. |
|
Thanks for the direct feedback. I have shortened the description and confined this revision to the requested test consolidation; the production fix is still one line. The new Linux head/main validation passes, including the negative control. These PRs were based on existing issues; I do not have a specific first-hand workflow report to add, and will not present them as one. I will keep further replies concise and will not start additional pprof backlog work without a maintainer go-ahead. |
| compareGraphs(t, buf.Bytes(), "compose7.dot") | ||
|
|
||
| // Backslashes are filename characters only on Unix. | ||
| if filepath.Separator != '\\' { |
There was a problem hiding this comment.
We have the TestEscapeForDot() test that checks the helper for all supported characters.
Maybe in this test it's enough to just test with the double quote? Other characters are supported because the common helper is used, so I think this code / case do not add much.
There was a problem hiding this comment.
Done in e2366c6: I kept only the quoted-filename regression in the existing compose/golden test and removed the additional newline/backslash cases. The helper tests are unchanged. Full Linux validation passes on head and main; the tests-only negative control fails the filename golden on main while TestEscapeForDot still passes. Run.
SG - thanks! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1032 +/- ##
==========================================
+ Coverage 73.42% 73.43% +0.01%
==========================================
Files 46 46
Lines 7936 7936
==========================================
+ Hits 5827 5828 +1
+ Misses 2086 2085 -1
Partials 23 23 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks for the review and approval. I have left the reviewed head |
Source filenames containing double quotes can break the quoted DOT node label at file/line granularity. Apply the existing DOT escaping after taking the basename.
The existing
TestComposeWithNamesThatNeedEscapingnow includes one quoted filename with a line/column suffix and uses its existing golden. I removed the additional newline/backslash filename cases as requested;TestEscapeForDotalready covers the helper's supported characters. The production fix remains one line.Addresses the filename portion of #1020; sample-tag escaping is separate (#1008).
Current head
e2366c6was validated against main7bae8d8cin this Linux run: the unchanged fulltest.shand fresh root/browser race tests pass on both trees. Chrome tests execute without browser skips; four existing root platform skips remain on both. With only the test/golden changes applied to main, the quoted-filename golden fails while all helper cases pass.Windows graph tests, graph race tests, vet and formatting pass. Earlier full-Windows PE/DWARF failures also occurred on main and are not claimed fixed. This is author validation, not the full upstream OS/Go matrix; upstream CI authorization and review remain required.