Guard timezone test conventions and make fixture semantics explicit - #3090
Chris0Jeky wants to merge 2 commits into
Conversation
Address #3013 without changing app clocks or mutation configuration. Add a scoped AST convention guard, host-independent ordering assertions, overlap/mixed-Date regressions and explicit execution limits.
|
@codex review Please inspect literal-call detection and the single scoped exception, host-independent fake-timer ordering, unchanged DST algorithm, mixed-Date assertions and Vite/TypeScript compatibility. Local standalone probes are documented separately from absent Vitest execution; exact-head hosted qualification and both pool checks remain required. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 062a35e325
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // No node:fs dependency in the Vitest/browser type-check project. Source parsing | ||
| // ignores instrumentation/layout changes; unlike a facade regex, it reads calls. | ||
| const sources = import.meta.glob('../**/*.{ts,tsx,js,mjs}', { |
There was a problem hiding this comment.
If a root-level unit spec such as tests/demo-run.spec.ts adds vi.stubEnv('TZ', ...), this guard will not detect it: the glob is resolved from src/tests/guards and only covers src/tests/**, while vitest.config.ts includes the root tests/*.spec.ts files in ordinary Vitest runs. This allows runnable tests outside src/tests to bypass the claimed convention and single-exception invariant, so the guard should also glob the root unit-test specs with suitable exclusions and a sentinel.
Useful? React with 👍 / 👎.
Address the first review without broadening into E2E/visual suites. Record the initial hosted ordering failure honestly; do not skip or weaken it.
|
Addressed discussion_r3998086412 in f4eaf21: the literal-call guard now covers root unit-test sources, matches the current Vitest E2E/visual/generated-worker exclusions, and requires a root-unit sentinel. A same-title call in a root test is not exempt. This does not repair the separate fake-timer ordering failure. Initial Linux qualification reported 6912 passed/3 skipped/1 failed, with the guard 16/16 passing; the latest head again passes lint/typecheck/build but fails frontend tests on both OSes. The trace excerpt differs from the fetched head/merge helper body, so no unsupported root-cause claim or test weakening was made. Keep this draft blocked pending a clean exact-head reproduction. The PR body and design note retain the failed evidence and exact next commands. @codex review |
Chris0Jeky
added
frontend
ChatGPT Codex Connector
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4eaf21a0a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| '../**/*.{ts,tsx,js,mjs}', | ||
| '../../../tests/**/*.{ts,tsx,js,mjs}', |
There was a problem hiding this comment.
Fresh evidence after the root-test fix is src/store/telemetryStore.consent.spec.ts: it is a runnable Vitest spec because vitest.config.ts retains default test discovery and only adds exclusions, but these globs cover only src/tests/** and root tests/**. A vi.stubEnv('TZ', ...) added to that existing colocated spec would therefore bypass the convention and single-exception invariant while the ordinary frontend suite still executes it; include colocated specs and add this file as a sentinel.
Useful? React with 👍 / 👎.
1 participant
Outcome and current blocker
Refs #3013. Draft: do not merge. The hosted frontend test step is still failing.
Keep the existing timezone fixture algorithm, pin its New York/Berlin overlap choices, and replace the host-dependent fake-timer ordering assertion with a captured-host comparison and a deliberately different installed zone. Add a mixed-Date regression showing constructors, zone-less parsing and local setters retain host behavior.
The test-only TypeScript AST guard finds literal TZ stubs without mistaking documentation or fixture strings for executed code. It requires exactly one scoped exception for the existing helper self-test, not a whole-file exemption. Computed keys, aliases and process.env writes remain explicit non-goals. No production runtime, dependencies, package files, CI workflows or mutation configuration change.
Review correction
Head
f4eaf21a0a5089a8640736626ce2cf92379a111aaddresses discussion_r3998086412: include roottestsunit sources as well assrc/tests, matching Vitest's E2E/visual/generated-worker exclusions, and add a root-unit sentinel. The ordering assertion is not skipped, weakened or claimed fixed by this coverage correction.Actual qualification
062a35e...: lint/typecheck/build passed on both OSes; Linux full suite reported 6,912 passed, three skipped and one failure in the fake-timer ordering test. The source guard passed 16/16. The trace reportedNativeDateTimeFormat is not defined; its displayed source excerpt does not match the fetched head/merge helper body, so no root cause or repair is inferred.f4eaf21...: lint/typecheck/build again passed on both OSes; frontend tests failed on both OSes. The scan follow-up therefore is not a complete qualification. No full green verdict is claimed.Next action: reproduce the failing ordering case on a clean exact-head checkout and retain the actual transformed-source/stack evidence; run both default and threads pools and the full frontend gate. The design note records exact commands, the failure and limitations. #3009/#3040 and #3038 remain separate mutation-runner work.
Isolation
Real parent
2cdc4525766101211fe04787e23bfb46b6aa4011; three original changed-file blobs matched that main. Surrounding local files are the older uploaded snapshot, not a claimed exact-main checkout. Six files only; independent of #3091 and active relation/editor/export/audit work.Canonical STATUS/MASTERPLAN and OUTSTANDING_TASKS.md human device/keyboard, screen-reader, translation, provider, release/hosting and CI-control decisions remain unchanged. No merge or deployment.