Skip to content

fix(cli): add V2 asset support for logs and check-run-data retrieval [AI-61]#1248

Merged
martzoukos merged 2 commits intomainfrom
martzoukos/fix-failing-e2e-tests
Mar 9, 2026
Merged

fix(cli): add V2 asset support for logs and check-run-data retrieval [AI-61]#1248
martzoukos merged 2 commits intomainfrom
martzoukos/fix-failing-e2e-tests

Conversation

@martzoukos
Copy link
Copy Markdown
Contributor

@martzoukos martzoukos commented Mar 6, 2026

Summary

  • Add V2 asset handling to packages/cli/src/rest/assets.ts so that checkly test --verbose displays logs and check-run-data for accounts using the LEGACY_RUNNER_ASSETS_V2 feature flag
  • Fix config.get crash in empty-account e2e tests by guarding with config.has

Problem

monorepo#596 (feat: add support for v2 assets for BROWSER/MULTI_STEP checks) changed how runners store check result assets. V1 uploaded each asset as a separate S3 file (logPath, checkRunDataPath, etc.). V2 bundles everything into a single ZIP archive with byte-range offsets per entry.

The runner side was updated but the CLI was not. The getAssets method in assets.ts only handled V1 and V3 formats — when it received a V2 asset, key stayed undefined and the method silently returned nothing. This meant npx checkly test --verbose showed no logs or check-run-data for BrowserCheck and MultiStepCheck runs on affected accounts.

The V2 types (CheckResultAssetsV2, isCheckResultAssetsV2) were already defined in the file but never wired into the retrieval logic.

Fix

Added a getAssetsV2 method that:

  1. Looks up the requested entry by name in the entries array (logs.txt for logs, check-run-data.json for check-run-data)
  2. Fetches a presigned S3 URL via the existing /redirect?link=true endpoint
  3. Uses an HTTP Range request to fetch just the needed bytes from the ZIP
  4. Parses and returns the JSON

@martzoukos martzoukos requested a review from sorccu March 6, 2026 16:40
@martzoukos
Copy link
Copy Markdown
Contributor Author

sweating-nervous

Ok, let's wait for the tests to pass in the PR, first. 😅

Copy link
Copy Markdown
Member

@sorccu sorccu left a comment

Choose a reason for hiding this comment

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

Those failures indicate an incompatible backend change. If accidental, this PR is not needed and the backend needs to be fixed. If on purpose, then we need to rethink these tests.

@martzoukos
Copy link
Copy Markdown
Contributor Author

Those failures indicate an incompatible backend change. If accidental, this PR is not needed and the backend needs to be fixed. If on purpose, then we need to rethink these tests.

Fixing the source is definitely the way to go, if the tests broke suddenly and recently. Thank you for researching that.

Spiros Martzoukos and others added 2 commits March 9, 2026 13:18
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@martzoukos martzoukos force-pushed the martzoukos/fix-failing-e2e-tests branch from b47a691 to 0e4b443 Compare March 9, 2026 11:27
@martzoukos martzoukos requested a review from sorccu March 9, 2026 11:37
@martzoukos
Copy link
Copy Markdown
Contributor Author

martzoukos commented Mar 9, 2026

Hello again @sorccu ,

armed with more context, I come back with new changes:

  • I kept the packages/cli/e2e/__tests__/checks-empty-account.spec.ts changes for better handling of the case when there are no CHECKLY_EMPTY_API_KEY and CHECKLY_EMPTY_ACCOUNT_ID env vars. The code currently throws and this is reported as a failed test in CI. The reason why it might have not been picked up yet in the PRs is because these vars are present in Github Actions (but they might throw and cause some confusion in local runs).
  • I replaced the previous test.spec.ts changes with the ones in packages/cli/src/rest/assets.ts. I'm obviously missing some context here but it seems that with a recent change on how we handle the fetching of the playwright tests' logs from S3 for accounts that are have the LEGACY_RUNNER_ASSETS_V2 flag enabled, we forgot to reflect this change in the CLI. With the help of claude, I made a addition for this, but I would appreciate a second set of eyes on this.

The tests seem to go green (save for the Windows one) with minimal changes to the cli tests code, so I think that might have a good solution on our hands.

Let me know what you think.

@martzoukos martzoukos enabled auto-merge (squash) March 9, 2026 11:38
@martzoukos martzoukos removed the request for review from sorccu March 9, 2026 12:43
@MegaMaddin
Copy link
Copy Markdown
Member

@martzoukos martzoukos changed the title fix(cli): fix failing e2e tests to assert on static strings fix(cli): fix failing e2e tests to assert on static strings [AI-61] Mar 9, 2026
@sorccu
Copy link
Copy Markdown
Member

sorccu commented Mar 9, 2026

Can you update the title and description to match the latest version of the PR?

@martzoukos martzoukos changed the title fix(cli): fix failing e2e tests to assert on static strings [AI-61] fix(cli): add V2 asset support for logs and check-run-data retrieval [AI-61] Mar 9, 2026
@martzoukos martzoukos merged commit 6049afa into main Mar 9, 2026
4 checks passed
@martzoukos martzoukos deleted the martzoukos/fix-failing-e2e-tests branch March 9, 2026 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants