test(profiles): pin profile chip↔dropdown source-of-truth invariant (#3635, stacked on #3637) - #3639
Conversation
…#3635) Adds a standing invariant guard generalizing the #3635 fix: the profile chip (syncTopbar/ui.js) and the profile dropdown's active/checkmark row (renderProfileDropdown/panels.js) are two renderings of 'which profile is active' and must resolve from the same source of truth (S.activeProfile). #3331 split them — the chip was pointed at S.session.profile while the dropdown kept reading S.activeProfile — so opening a cross-profile session made the switcher trigger disagree with the menu it opens (#3635). A purely additive, deterministic test (no browser, runs in the normal pytest suite) that fails the instant the two surfaces key on different variables again: - test_chip_keys_on_active_profile — chip reads S.activeProfile (ui.js) - test_dropdown_active_row_keys_on_active_profile — dropdown active row reads S.activeProfile (panels.js) - test_chip_and_dropdown_share_source_of_truth — cross-file: both agree Stacked on the #3635 fix (PR #3637). No production code change. Refs #3635
|
| Filename | Overview |
|---|---|
| tests/test_issue3635_profile_chip_active.py | Adds TestProfileSwitcherSourceOfTruthInvariant with three invariant tests; the cross-file test's chip-side positive check is satisfied by comment text rather than the actual assignment expression, and the assignment-locating regex has a 120-char gap limit that could produce misleading failures on reformatting. |
Reviews (1): Last reviewed commit: "test(profiles): pin profile chip<->dropd..." | Re-trigger Greptile
| chip_ok = "S.activeProfile" in chip_body and \ | ||
| "(S.session&&S.session.profile)||S.activeProfile" not in chip_body |
There was a problem hiding this comment.
chip_ok positive check is satisfied by comment text, not the assignment
"S.activeProfile" in chip_body is trivially True on the current code because syncTopbar() contains inline comments (lines ~5907-5908 of ui.js) that spell out S.activeProfile in explanatory prose — e.g., // It must therefore reflect S.activeProfile, NOT the loaded session's profile. The substring match hits the comment before it ever reaches the actual assignment expression. If a future change re-points the chip to S.session?.profile || S.activeProfile (or any novel form that doesn't match the negative pattern), chip_ok remains True and the test passes silently, exactly contradicting the docstring that says "the switcher trigger and the menu it opens can never again silently disagree."
The two focused tests (test_chip_keys_on_active_profile with its findall regex, and test_both_chip_setters_consistent) already pin the actual assignment expressions and would catch this class of regression — but test_chip_and_dropdown_share_source_of_truth overstates its own guarantee on the chip side. Consider anchoring chip_ok to the same regex-based assignment extraction used in test_chip_keys_on_active_profile rather than a bare in chip_body substring test.
| assignments = re.findall(r"profileChipLabel'\);[\s\S]{0,120}?\.textContent=([^;]+);", body) | ||
| assert assignments, "no profileChipLabel assignment found in syncTopbar()" | ||
| for expr in assignments: | ||
| assert "S.activeProfile" in expr and "S.session.profile" not in expr, ( | ||
| "profile chip (switcher trigger) must resolve from S.activeProfile, " | ||
| "not the loaded session's profile: " + expr.strip() | ||
| ) |
There was a problem hiding this comment.
120-char window in the regex could produce a misleading failure on reformatting
profileChipLabel'\);[\s\S]{0,120}?\.textContent= caps the gap between the $('profileChipLabel') call and the .textContent= assignment at 120 characters. Right now the gap is roughly 35 characters (\nif(_profileLabel) _profileLabel), so it works. But if someone adds an inline guard, splits the if-condition, or inserts a short comment between the two statements, re.findall could return [] — causing the assertion assert assignments to fire with "no profileChipLabel assignment found in syncTopbar()" rather than any message about source-of-truth violations. The failure diagnostic would be actively misleading.
…from #3639) (#3644) Test-only. Adds TestProfileSwitcherSourceOfTruthInvariant generalizing the #3635 fix so the chip + dropdown can't re-split their source of truth (both must read S.activeProfile). Rebased onto current master — the original #3639 branch was stacked on the pre-squash #3637 and would have reverted ~5 shipped releases (IF/IG/IH) if merged as-is; this carries ONLY the +74-line test delta. Co-authored-by: nesquena-hermes <[email protected]> Co-authored-by: nesquena <nesquena@users.noreply.github.com>
…rebased from nesquena#3639) (nesquena#3644) Test-only. Adds TestProfileSwitcherSourceOfTruthInvariant generalizing the nesquena#3635 fix so the chip + dropdown can't re-split their source of truth (both must read S.activeProfile). Rebased onto current master — the original nesquena#3639 branch was stacked on the pre-squash nesquena#3637 and would have reverted ~5 shipped releases (IF/IG/IH) if merged as-is; this carries ONLY the +74-line test delta. Co-authored-by: nesquena-hermes <[email protected]> Co-authored-by: nesquena <nesquena@users.noreply.github.com>
…rebased from nesquena#3639) (nesquena#3644) Test-only. Adds TestProfileSwitcherSourceOfTruthInvariant generalizing the nesquena#3635 fix so the chip + dropdown can't re-split their source of truth (both must read S.activeProfile). Rebased onto current master — the original nesquena#3639 branch was stacked on the pre-squash nesquena#3637 and would have reverted ~5 shipped releases (IF/IG/IH) if merged as-is; this carries ONLY the +74-line test delta. Co-authored-by: nesquena-hermes <[email protected]> Co-authored-by: nesquena <nesquena@users.noreply.github.com>
Summary
Stacked on #3637 (base =
fix/3635-profile-chip-active). This PR is test-only — no production code — and should merge after #3637. Once #3637 merges to master, GitHub will retarget this tomasterautomatically; the diff is purely the new test.Adds a standing source-of-truth invariant test that generalizes the #3635 fix so the regression can't silently come back.
Why
#3635 was a UI source-of-truth divergence: the composer profile chip (
syncTopbar()inui.js) and the profile dropdown's active/checkmark row (renderProfileDropdown()inpanels.js) are two renderings of the same fact — which profile is active — but #3331 pointed the chip atS.session.profilewhile the dropdown kept readingS.activeProfile. Opening a cross-profile session made the switcher trigger disagree with the menu it opens, and misrepresented where the next message would route.#3637 fixes the chip. This PR pins the invariant so a future change can't re-split the two surfaces:
What
tests/test_issue3635_profile_chip_active.pygains aTestProfileSwitcherSourceOfTruthInvariantclass (deterministic JS-source assertions, no browser, runs in the normal pytest suite):test_chip_keys_on_active_profile— the chip (ui.jssyncTopbar) resolves fromS.activeProfile, neverS.session.profiletest_dropdown_active_row_keys_on_active_profile— the dropdown active row (panels.jsrenderProfileDropdown) resolves fromS.activeProfiletest_chip_and_dropdown_share_source_of_truth— cross-file check that both key onS.activeProfileTesting
tests/test_issue3635_profile_chip_active.py: 6 passed (3 from fix(profiles): profile chip reflects active profile, not session's (#3635) #3637 + 3 new).syncTopbar()+renderProfileDropdown()and asserts the chip equals the dropdown's active row after a cross-profile session load) was added to the private QA harness and RED/GREEN-validated: it fails on the pre-fix(profiles): profile chip reflects active profile, not session's (#3635) #3637 buggy code (chip='profileA'vsdropdown='profileB') and passes on the fix.Refs #3635