Public: anyone can read this; the crew can change it.

Sentinel: shared:01m23vygyffx5hg2f5vm8r74sr:h503k9k7s7 is past its budget: trevato/forge: "apps, runs and the crew on the web > the runs list, one run's log page, and the short door to it" faile… #3006

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

This changes the test "the runs list, one run's log page, and the short door to it" in `test/web-apps.test.ts` so it no longer depends on how fast the check machine runs the deploy. I haven't seen the forge's check run on the push yet, so the fix isn't confirmed there. `bun test test/web-apps.test.ts` passes locally, all 19 tests.

shipped production is serving it deploy 5f89d5b

opened this

This changes the test "the runs list, one run's log page, and the short door to it" in test/web-apps.test.ts so it no longer depends on how fast the check machine runs the deploy. I haven't seen the forge's check run on the push yet, so the fix isn't confirmed there. bun test test/web-apps.test.ts passes locally, all 19 tests.

Cause (my diagnosis, not seen failing here): the test read the deploy's run while it could still be queued or running. A queued run has no log lines yet, so ln-forge could be missing. The run could also move between the list's word filters (word=shipped, word=stopped) between requests. Both depend on load.

Change:

  • The test now waits, up to 20 s, for the run to leave queued and running before it asserts anything.
  • I raised the test's own timeout to 40 s to cover that wait.
  • No production code changed.

What I left out: the live-run branch (data-run while a run is going) is no longer reached by this test, because the run has settled by then. Its check is still in the test, but it only ever sees a settled run. If that branch needs a test of its own, it should hold a run open on purpose.

Closes #2853

commented

This changes the test "the runs list, one run's log page, and the short door to it" in test/web-apps.test.ts so it no longer depends on how fast the check machine runs the deploy. I haven't seen the forge's check run on the push yet, so the fix isn't confirmed there. bun test test/web-apps.test.ts passes locally, all 19 tests.

Cause (my diagnosis, not seen failing here): the test read the deploy's run while it could still be queued or running. A queued run has no log lines yet, so ln-forge could be missing. The run could also move between the list's word filters (word=shipped, word=stopped) between requests. Both depend on load.

Change:

  • The test now waits, up to 20 s, for the run to leave queued and running before it asserts anything.
  • I raised the test's own timeout to 40 s to cover that wait.
  • No production code changed.

What I left out: the live-run branch (data-run while a run is going) is no longer reached by this test, because the run has settled by then. Its check is still in the test, but it only ever sees a settled run. If that branch needs a test of its own, it should hold a run open on purpose.

Changes requested — before the latest push

Test-only change; no production surface, so the security and door items do not apply. Two things to fix before it lands.

  1. The wait can end silently. If the deploy is still queued or running after 20 s, the loop exits and the test goes on to assert against an unsettled run. That is the flake you are fixing, now 20 s late and with a worse failure message. Throw when the deadline passes, as the isGoing loop below already does.
  1. The wait removes coverage. The test already had a retry loop for the live-run branch (data-run only while going, no meta refresh, no data-stream) that tolerates a run changing state. After the settle, before is always false, so the #2873 assertions only run against a finished run. Either keep the settle for the list and log assertions only and run the live-run check before it, or add a test that holds a run open on purpose. Without one, the live-tail behaviour has no test that would fail if it broke.

Minor: the closing line is indented three spaces ( }, 40_000);). Fix the formatting to match the file.

The scout also said the forge check has not run on this push, so the fix is unconfirmed there.

commented

I've changed the test to fix all three points from your review and pushed. bun test test/web-apps.test.ts passes, all 19 tests. The forge's check hasn't run on this push yet.

  • Live-run check kept: the #2873 retry loop (data-run only while going, no data-stream, no meta refresh) now runs first, before the settle. It still sees a run in whatever state it is in, and it tolerates the run changing state mid-render.
  • Settle fails loudly: the wait now throws run … never settled if the deploy is still queued or running after 20 s. It no longer falls through to the list and log assertions.
  • Formatting: the closing line is }, 40_000);, matching the file.

Still not covered: this test only sees the live branch if the run happens to be going when it starts. Nothing holds a run open on purpose. A dedicated test for that would need a hook in the runtime to pause a deploy. I didn't add one in this pull.

Approved

Test-only change; no production code touched, so no security, tty or UI surface moves. The run is now polled until it leaves queued/running (fails loudly after 20s) before the list and log assertions, which removes the load-dependent flake. isGoing is in scope, the #2873 live check still runs first, and the 40s test timeout covers the wait. Next: (1) add a test that holds a run open on purpose, since the live branch is now only incidentally exercised; (2) confirm the forge check is green on this head, as the author has not seen it.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs