Skip to content

test: pin platform_fingerprint degrade paths (PRPUNDIT-12) - #1327

Merged
chaojhou merged 4 commits into
AMD-AGI:mainfrom
jiagaoxiang:testgap/PRPUNDIT-12
Sep 1, 2026
Merged

test: pin platform_fingerprint degrade paths (PRPUNDIT-12)#1327
chaojhou merged 4 commits into
AMD-AGI:mainfrom
jiagaoxiang:testgap/PRPUNDIT-12

Conversation

@jiagaoxiang

Copy link
Copy Markdown
Collaborator

Summary

  • Exercises platform_fingerprint three-tier degrade: probe None → unavailable, outer exception → status=error, and per-block gpu/stack status=error.

Closes test gap PRPUNDIT-12.

Test plan

  • PYTHONPATH=src pytest src/hyperloom/common/tests/test_platform_probe.py -k platform_fingerprint

Exercise unavailable (probe None), outer status=error, and per-block gpu/stack degrade.
@jiagaoxiang
jiagaoxiang requested a review from a team as a code owner August 28, 2026 07:13

@zoroyihan7 zoroyihan7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks — all three degrade tiers are reached and the assertions are on the real returned dict (including the exact "no host CPU sysfs on this machine" literal), no mock-asserts-the-mock anywhere.

Two things mutation testing turned up.

1. Per-block independence isn't actually pinned. test_platform_fingerprint_gpu_and_stack_blocks_degrade breaks both amdgpu_device_count and detect_stack_fingerprint in the same call, so it can never observe a healthy block surviving its sibling's failure — which is the "the rest of the record survives" half of the contract. Mutating the stack handler to also overwrite record["gpu"] and record["cpu"] leaves all 15 tests green. Worse, swapping which key each handler writes (GPU handler setting record["stack"] and vice versa) is undetectable, and in the single-block-failure case silently drops the "gpu" key entirely.

Splitting into two cases — gpu raises / stack healthy, and stack raises / gpu healthy — and asserting the surviving block's real content in each (e.g. got["stack"] != {"status": "error"}, got["gpu"]["gfx_arch"] present) closes both.

2. The outer net is pinned for only one exception type. Narrowing except Exception as exc to except RuntimeError as exc stays green, because the new test happens to raise RuntimeError. An OSError/AttributeError escaping probe_cpu_platform would then propagate to the crash-path caller with no test noticing — and per exporter.py:1010-1013 there's no second net there, so reports/final.json would be lost for a run that already died. Raising a non-RuntimeError in one of the cases fixes it.

Smaller ones:

  • "multi_node_session": multi_nodebool(multi_node) stays green, which destroys the docstring's explicit None-vs-False distinction ("None records that nobody established it ... rather than an unearned False") — no test calls platform_fingerprint with multi_node left unset on an ok record.
  • No test covers the all-healthy status="ok" record, so the gpu sub-dict shape (host_count / gfx_arch / amdgpu_driver, the "unknown" fallbacks) has no pin — deleting or renaming any of those keys keeps the suite green. Deleting the "host": socket.gethostname() line is green too.
  • The new tests monkeypatch module globals, which the module docstring (lines 24-25) discourages in favour of the injectable root. It's justified here — platform_fingerprint takes no root and the error tiers aren't reachable through a fake sysfs tree — but a one-line comment saying so would stop the next reader "fixing" it.

@zoroyihan7 zoroyihan7 added the skip-e2e-test It's a PR that doesn't need to be e2e tested label Aug 31, 2026
jiagaoxiang and others added 2 commits August 31, 2026 19:48
…erprint

1. Change outer test from RuntimeError → OSError to pin the broad except Exception
   net; narrowing it to RuntimeError would silently let OSError/AttributeError escape.
2. Split the combined GPU+stack-fail test into two independent cases:
   - GPU raises (OSError) / stack returns real dict  → assert stack == fake_stack
   - Stack raises (RuntimeError) / GPU returns count → assert 'gfx_arch' in gpu block
   Each case proves the healthy sibling's content survives its partner's failure.

Addresses zoroyihan7 review on PR AMD-AGI#1327.
The degrade split already pinned per-block independence. This adds the
all-healthy status=ok gpu sub-dict (host_count/gfx_arch/amdgpu_driver,
including the "unknown" fallbacks), host=gethostname(), and the
docstring's multi_node None-vs-False distinction. Comments why these
cases monkeypatch module globals.
@jiagaoxiang

Copy link
Copy Markdown
Collaborator Author

Split GPU-raises/stack-healthy vs stack-raises/GPU-healthy (asserting the surviving block's real content), and the outer net now raises OSError. Added the all-healthy status=ok gpu sub-dict (host_count / gfx_arch / amdgpu_driver, including "unknown" fallbacks), host=gethostname(), and multi_node is None vs False. Comment on the tests explains why these cases monkeypatch module globals (platform_fingerprint has no injectable root).

@zoroyihan7 zoroyihan7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed. Splitting into two single-block-failure cases is exactly what was needed — the surviving-block content assertions are what make the key-swap mutations detectable.

Five mutations, all red now:

  • except Exceptionexcept RuntimeError on the outer net (L263) — red
  • gpu handler writing record["stack"] instead of record["gpu"] (L256) — red
  • stack handler writing record["gpu"] instead of record["stack"] (L261) — red
  • multi_nodebool(multi_node) (L236) — red
  • host key renamed (L235) — red

17 passed. LGTM.

@jiagaoxiang
jiagaoxiang enabled auto-merge (squash) September 1, 2026 03:35
@zoroyihan7 zoroyihan7 added the retest Re-run E2E smoke without a new commit (runs once) label Sep 1, 2026
@chaojhou
chaojhou disabled auto-merge September 1, 2026 09:42
@chaojhou
chaojhou merged commit 43a949d into AMD-AGI:main Sep 1, 2026
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

retest Re-run E2E smoke without a new commit (runs once) skip-e2e-test It's a PR that doesn't need to be e2e tested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants