Skip to content

internal/graph: escape source filenames in DOT labels - #1032

Open
jabrailkhalil wants to merge 13 commits into
google:mainfrom
jabrailkhalil:fix/dot-source-filenames
Open

jabrailkhalil wants to merge 13 commits into
google:mainfrom
jabrailkhalil:fix/dot-source-filenames

Conversation

@jabrailkhalil

@jabrailkhalil jabrailkhalil commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Source filenames containing double quotes can break quoted DOT node labels at file/line granularity. I apply the existing DOT escaping after taking the basename. The existing TestComposeWithNamesThatNeedEscaping table adds one quoted filename with a line/column suffix and updates its golden. TestEscapeForDot already covers the helper's other supported characters; the production fix is one line.

Addresses the filename portion of #1020; sample-tag escaping is separate (#1008).

Current head 37811fb215a47125a910dee0aef90928cbc7fefe passes upstream CI, including the Linux, macOS and Windows matrix, and the security checks. Earlier author-only validation remains linked in the discussion and is not presented as a run of this head.

Comment thread internal/graph/dotgraph_test.go Resolved
Comment thread internal/graph/dotgraph_test.go Resolved
@jabrailkhalil

Copy link
Copy Markdown
Contributor Author

I consolidated the filename regressions into the existing compose-test table as requested and updated this branch from main. On current head fdc18ddac53f, both the root and browsertests modules pass go test ./... -count=1 on Linux with Go 1.26.7. The consolidated filename cases also fail when the production escaping is reverted. The current upstream CI/security workflows are waiting for maintainer approval; could you approve those runs when reviewing this revision?

Comment thread internal/graph/dotgraph_test.go Resolved
@jabrailkhalil

Copy link
Copy Markdown
Contributor Author

Integrated current main 7bae8d8c in 95895448 to clear BEHIND without adding feature scope.

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?

@jabrailkhalil

jabrailkhalil commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor Author

Validated unchanged head 95895448dfd405a9b55bd5c6c1286ff75b846bf1 against main 7bae8d8c4c9ebc3886a79f796fbde65b7e1316f8 in an isolated author-fork Linux run. Ubuntu 24.04 / Go 1.26.8 / Chrome 154, with Graphviz and LLVM available. The complete, unmodified test.sh passed, including both modules, race/coverage, staticcheck 2026.2.1, the upstream golangci-lint v2.14.0 commands, vet and formatting. Fresh go test -race -count=1 -json ./... also passed in each module; no browser test was skipped. The four pre-existing root test/subtest platform skips are retained on both head and main.

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 test.sh are byte-identical between the compared trees. The diagnostic workflow exists only on a separate branch in my fork; this PR head, assertions, deadlines and upstream workflow were not changed. This is author validation on one Linux/Go version, not the full upstream OS/Go matrix or a replacement for required Google CI/review; the earlier Windows failures below are not relabeled as passing.

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?

Comment thread internal/graph/dotgraph_test.go Resolved
@aalexand

aalexand commented Oct 11, 2026 •

Copy link
Copy Markdown
Collaborator

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

@jabrailkhalil

Copy link
Copy Markdown
Contributor Author

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.

Comment thread internal/graph/dotgraph_test.go Resolved
@aalexand

Copy link
Copy Markdown
Collaborator

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.

SG - thanks!

@codecov-commenter

codecov-commenter commented Oct 12, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.43%. Comparing base (1facf8f) to head (37811fb).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jabrailkhalil

jabrailkhalil commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks for the review and approval. Current head is 37811fb215; its upstream Linux, macOS and Windows checks are green. I verified and resolved the five addressed review threads. Could a maintainer complete the merge when convenient? I am keeping the scope unchanged.

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.

3 participants