Skip to content

Cleanup and simplify hang dump implementation#7700

Merged
Youssef1313 merged 4 commits intomainfrom
dev/ygerges/cleanup-hangdump
Apr 9, 2026
Merged

Cleanup and simplify hang dump implementation#7700
Youssef1313 merged 4 commits intomainfrom
dev/ygerges/cleanup-hangdump

Conversation

@Youssef1313
Copy link
Copy Markdown
Member

No description provided.

Copilot AI review requested due to automatic review settings April 9, 2026 11:11
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR simplifies the hang dump named-pipe environment variable naming by removing the application-path-based hash and using only a random per-run suffix, reducing coupling to ITestApplicationModuleInfo.

Changes:

  • Remove the FNV-1a string hash helper previously used for pipe env-var key generation.
  • Simplify HangDumpConfiguration.PipeNameKey to TESTINGPLATFORM_HANGDUMP_PIPENAME_<namedPipeSuffix> and align the activity indicator’s lookup logic.
  • Simplify hang dump extension setup by removing unnecessary CurrentTestApplicationModuleInfo construction.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/Platform/Microsoft.Testing.Extensions.HangDump/Helpers/FNV1HashHelper.cs Removes the now-unused hash helper.
src/Platform/Microsoft.Testing.Extensions.HangDump/HangDumpExtensions.cs Stops constructing module-info solely for hashing; uses simplified configuration ctor.
src/Platform/Microsoft.Testing.Extensions.HangDump/HangDumpConfiguration.cs Updates pipe env-var key generation to use only the random suffix.
src/Platform/Microsoft.Testing.Extensions.HangDump/HangDumpActivityIndicator.cs Updates pipe env-var lookup to match the simplified naming scheme.

Copilot AI review requested due to automatic review settings April 9, 2026 11:55
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 9, 2026 12:01
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Copy link
Copy Markdown
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean, well-scoped simplification. The old hash-based env var naming (TESTINGPLATFORM_HANGDUMP_PIPENAME_<hash>_<suffix> + a separate suffix env var) was unnecessary complexity — the pipe name itself is already a unique GUID. Collapsing to a single TESTINGPLATFORM_HANGDUMP_PIPENAME env var, removing HangDumpConfiguration, FNV_1aHashHelper, and ExitSignalActivityIndicatorTaskRequest removes 3 files and ~73 net lines while making the code easier to follow. The DESIGN.md is a valuable addition.

One minor nit about inconsistent GetPipeName overload usage (see inline). No blocking issues — all changed types are internal, so no public API surface concerns.

@Youssef1313 Youssef1313 merged commit 1b51e3e into main Apr 9, 2026
15 checks passed
@Youssef1313 Youssef1313 deleted the dev/ygerges/cleanup-hangdump branch April 9, 2026 21:13
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