Repository navigation
Make timezone tests race-safe and complete smoke verification reproducible - #642
Conversation
|
🤖 @codex review Please focus on preserving local-day/all-day behavior while removing global timezone mutation from tests. Production still passes the machine's location. The longer whole-suite smoke deadline retains strict mode and every assertion; full SMTP verification made the old global five-minute budget expire during an otherwise healthy test. |
Codex Review Summary: ✅ CompletedThis 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. |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
🟢 Approved
No active review issues remain for the timezone-test isolation and smoke-suite documentation changes.
No open issues 🎉
What changed in this PR
Removes the timezone-test race exposed during cookie-auth removal verification and documents how to run the complete live smoke suite. Production timezone behavior remains unchanged; the broader account-timezone feature stays out of scope.
Changes:
- Pass explicit locations to private event helpers instead of mutating
time.Localin tests. - Preserve command-level ordering coverage and add east/west date-crossing and all-day cases.
- Document smoke-test prerequisites and extend the suite deadline from five to ten minutes without relaxing assertions or strict mode.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
tests/smoke/README.md |
Documents search, image-processing, SMTP, and sender prerequisites. |
Makefile |
Extends the smoke-test deadline to ten minutes. |
internal/cmd/events.go |
Accepts an explicit location for timed boundary formatting. |
internal/cmd/events_period.go |
Threads locations through sorting and day helpers; production uses time.Local. |
internal/cmd/events_period_test.go |
Removes global timezone mutation and expands timezone regression coverage. |
Review effort: Balanced
74fa587 to
d9753b9
Compare
|
🤖 @codex review Second review round: #641 is merged and this PR is rebased onto the updated main, inheriting cookie-auth removal and Go 1.27.4. The five-file patch is unchanged; range-diff shows only README context adjustments around the newly merged documentation. Please check the integration and retained timezone coverage. CI, local checks, shuffled race tests and the full strict live suite are rerunning. This PR remains unmerged. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Summary
Fix the intermittent timezone-test race seen while verifying cookie-auth removal #641, and make the complete live-test setup reproducible without skipping checks.
time.Localin tests. HTTP transport goroutines can read that global throughtime.Now, so the old test setup/cleanup raced with unrelated work.time.Local; command behavior is unchanged. Keep command-level ordering coverage and independently test positive/negative date crossings and unshifted all-day dates.Rebased onto main after #641 merged, inheriting cookie-auth removal and Go 1.27.4.
git range-diffconfirms the patch is unchanged apart from README context adjustments. Fresh verification is complete ond9753b9with Go 1.27.4: CI is green (21 checks pass, 4 skipped, none failing or pending), and local checks, three full shuffled race repetitions, and all 147 strict live tests pass.This does not implement the broader account-timezone/day-window feature in #563. No CLI commands/flags, SDK or API contracts change.
Verification
make check: pass.go test -race -shuffle=on -count=3 ./...: pass across the whole repository.make test-smoke: 147/147 top-level tests pass, zero skips, including SMTP capture and browser checks, in 343.0 seconds on the rebased head. The default ten-minute deadline is used; no tests or assertions are disabled.Related bootstrap fix / limits
Haystack's own setup currently starts Elasticsearch 7 even though its configuration expects ES8 on port 29208, then suppresses search-indexing errors. Companion haystack PR #9325 corrects that prerequisite, waits for service health and makes indexing failures visible; it does not change API behavior.
The previous Go security blocker is resolved by the Go 1.27.4 fix merged in #641 and inherited by this rebase; this PR's fresh Security check passes. No security gate is disabled or waived. This PR remains unmerged; no release or deployment performed.