Skip to content

feat: run e2e tests on vsc release #7190

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Patrick-Erichsen
Copy link
Collaborator

@Patrick-Erichsen Patrick-Erichsen commented Aug 15, 2025

Summary by cubic

Run VS Code E2E tests in the release workflow and add a reusable E2E workflow + composite action to remove duplicated CI logic. Releases are now validated by the same test matrix used in PRs.

  • New Features

    • Added composite action to build the VS Code extension, compile E2E tests, cache deps, and output a test-file matrix. Excludes SSH tests on forks.
    • Added reusable e2e-tests workflow that generates the matrix and runs tests for both e2e:ci:run and e2e:ci:run-yaml.
    • Hooked e2e-tests into the release workflow so publish waits for E2E to pass.
  • Refactors

    • Replaced bespoke PR E2E jobs with the reusable workflow, removing duplicate matrix/build steps.
    • Centralized secrets handling and is_fork logic in the reusable workflow.
    • Updated job dependencies so build and publish wait on vscode-e2e-tests.

@Patrick-Erichsen Patrick-Erichsen requested a review from a team as a code owner August 15, 2025 23:59
@Patrick-Erichsen Patrick-Erichsen requested review from tomasz-stefaniak and removed request for a team August 15, 2025 23:59
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Aug 16, 2025
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR refactors E2E test workflows by extracting common logic into reusable components. The changes improve maintainability and reduce duplication between main and PR workflows. However, there are several issues with the YAML syntax and potential workflow configuration problems that need to be addressed.


💡 To request a new detailed review, comment @continue-detailed-review

@@ -0,0 +1,58 @@
name: 'E2E Tests'
description: 'Reusable workflow for running VS Code E2E tests'

Choose a reason for hiding this comment

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

Remove 'description' field - it's not valid at the workflow level. Use 'name' only for workflow identification.

uses: actions/upload-artifact@v4
with:
name: vscode-extension-build-Linux
path: extensions/vscode/build

Choose a reason for hiding this comment

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

Missing newline at end of file. Add a newline character to follow YAML best practices.

command: ${{ matrix.command }}
ssh_key: ${{ secrets.GH_ACTIONS_SSH_TEST_KEY_PEM }}
ssh_host: ${{ secrets.GH_ACTIONS_SSH_TEST_DNS_NAME }}
is_fork: ${{ inputs.is_fork }}

Choose a reason for hiding this comment

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

Missing newline at end of file. Add a newline character to follow YAML best practices.

npm run e2e:compile
if [[ "${{ inputs.is_fork }}" == "true" ]]; then
# Exclude SSH tests for forks
FILES=$(ls -1 e2e/_output/tests/*.test.js | grep -v "SSH" | jq -R . | jq -s .)

Choose a reason for hiding this comment

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

Consider using 'shopt -s nullglob' before the ls command to handle the case when no test files match the pattern, preventing potential errors.

# Include all tests for non-forks
FILES=$(ls -1 e2e/_output/tests/*.test.js | jq -R . | jq -s .)
fi
echo "test_file_matrix<<EOF" >> $GITHUB_OUTPUT

Choose a reason for hiding this comment

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

Using heredoc syntax for GitHub outputs can be problematic with JSON. Consider using a single line: echo "test_file_matrix=$FILES" >> $GITHUB_OUTPUT

is_fork: ${{ inputs.is_fork }}

vscode-e2e-tests:
name: ${{ matrix.test_file || 'unknown' }} (${{ matrix.command }})

Choose a reason for hiding this comment

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

The fallback 'unknown' for matrix.test_file suggests potential issues. Consider adding validation to ensure test_file_matrix is properly populated.

@@ -135,10 +136,23 @@ jobs:
token: ${{ secrets.CI_GITHUB_TOKEN }}
repository: continuedev/continue

vscode-e2e-tests:
needs: check_release_name
if: needs.check_release_name.outputs.should_run == 'true' || github.event_name == 'workflow_dispatch'

Choose a reason for hiding this comment

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

Consider adding 'if: always()' or 'if: success()' to clarify when E2E tests should run in the main workflow.

run: |
node ./scripts/build-packages.js
cd extensions/vscode
npm ci

Choose a reason for hiding this comment

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

Add error handling: 'npm ci || exit 1' to ensure the workflow fails properly if npm install fails.

echo "EOF" >> $GITHUB_OUTPUT
env:
# https://github.com/microsoft/vscode-ripgrep/issues/9#issuecomment-643965333
GITHUB_TOKEN: ${{ inputs.ci-github-token }}

Choose a reason for hiding this comment

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

Add a comment explaining why CI_GITHUB_TOKEN is used here instead of the regular github-token input.

is_fork: ${{ github.event.pull_request.head.repo.fork == true || github.actor == 'dependabot[bot]' }}
uses: ./.github/workflows/e2e-tests.yml
with:
is_fork: ${{ github.event.pull_request.head.repo.fork == true || github.actor == 'dependabot[bot]' }}

Choose a reason for hiding this comment

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

Good refactoring! Consider adding a comment explaining that SSH tests are excluded for forks in the reusable workflow.

Copy link

Code Review Summary

✅ Strengths

  • Modularization: Great refactoring to extract e2e test logic into reusable workflows and actions
  • Caching Strategy: Comprehensive caching of npm dependencies and node_modules across different components
  • Fork Safety: Properly handles fork restrictions by excluding SSH tests when is_fork is true
  • Matrix Testing: Smart test file matrix generation that dynamically discovers test files
  • Artifact Management: Good use of artifacts to share build outputs between jobs

⚠️ Issues Found

Medium

  • Incorrect Workflow Trigger Type: The e2e-tests.yml file uses description: field which is invalid for workflow_call triggers. This should be removed as it only applies to workflow_dispatch triggers.

  • Missing Error Handling: In the composite action, when building packages fails, the workflow continues without proper error propagation. The shell scripts should use set -e or check exit codes.

  • Hardcoded Fallback PR Number: In main-build.yaml, there's a hardcoded fallback PR number (6723) which could cause confusion:

    PR_NUMBER=$(echo "$COMMIT_MSG" | grep -o "Merge pull request #[0-9]*" | grep -o "[0-9]*" || echo "6723")

Low

  • Redundant Output Debug: The "Debug Outputs" step in the composite action could be removed in production or gated behind a debug flag

  • Test File Discovery Improvements: The test file discovery uses basic ls and grep. Consider using find with more robust patterns for better reliability

💡 Suggestions

  • Add Workflow Documentation: Consider adding comments in the reusable workflow explaining the expected secrets and when to use it

  • Timeout Configuration: The hardcoded 15-minute timeout for e2e tests might need to be configurable for different test suites

  • Better Error Messages: When SSH tests are skipped for forks, consider logging a message to make it clear why certain tests were excluded

  • Consider Using GitHub Environments: For managing secrets like SSH keys, GitHub Environments could provide better security and audit trails

🚀 Overall Assessment

APPROVE

This is a well-structured refactoring that improves code reusability and maintainability. The extraction of e2e test logic into reusable workflows follows GitHub Actions best practices. The only critical issue is the incorrect workflow syntax which should be fixed before merging, but it's a simple fix. The changes demonstrate good understanding of GitHub Actions patterns and proper handling of security concerns around forks and secrets.


💡 To request a new review, comment @continue-general-review

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

cubic analysis

4 issues found across 4 files • Review in cubic

React with 👍 or 👎 to teach cubic. You can also tag @cubic-dev-ai to give feedback, ask questions, or re-run the review.

@@ -0,0 +1,58 @@
name: 'E2E Tests'
description: 'Reusable workflow for running VS Code E2E tests'
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the top-level 'description' key; it is not a valid workflow property and will cause workflow syntax validation errors

Prompt for AI agents
Address the following comment on .github/workflows/e2e-tests.yml at line 2:

<comment>Remove the top-level &#39;description&#39; key; it is not a valid workflow property and will cause workflow syntax validation errors</comment>

<file context>
@@ -0,0 +1,58 @@
+name: &#39;E2E Tests&#39;
+description: &#39;Reusable workflow for running VS Code E2E tests&#39;
+
+on:
</file context>

is_fork: ${{ github.event.pull_request.head.repo.fork == true || github.actor == 'dependabot[bot]' }}
uses: ./.github/workflows/e2e-tests.yml
with:
is_fork: ${{ github.event.pull_request.head.repo.fork == true || github.actor == 'dependabot[bot]' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

If this workflow runs on a push event (where the pull_request payload is absent), the expression throws because github.event.pull_request is undefined. Guard the access with a check on github.event_name first to avoid runtime evaluation errors.

Prompt for AI agents
Address the following comment on .github/workflows/pr-checks.yaml at line 198:

<comment>If this workflow runs on a push event (where the pull_request payload is absent), the expression throws because github.event.pull_request is undefined. Guard the access with a check on github.event_name first to avoid runtime evaluation errors.</comment>

<file context>
@@ -192,112 +192,15 @@ jobs:
           AZURE_OPENAI_GPT41_API_KEY: ${{ secrets.AZURE_OPENAI_GPT41_API_KEY }}
           VOYAGE_API_KEY: ${{ secrets.VOYAGE_API_KEY }}
 
-  vscode-get-test-file-matrix:
-    runs-on: ubuntu-latest
-    outputs:
-      test_file_matrix: ${{ steps.vscode-get-test-file-matrix.outputs.test_file_matrix }}
-    steps:
-      - uses: actions/checkout@v5
</file context>
Suggested change
is_fork: ${{ github.event.pull_request.head.repo.fork == true || github.actor == 'dependabot[bot]' }}
is_fork: ${{ (github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork == true) || github.actor == 'dependabot[bot]' }}

ssh_key:
description: 'SSH key for SSH tests'
required: false
ssh_host:
Copy link
Contributor

Choose a reason for hiding this comment

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

Input ssh_host is declared but not used anywhere in the action; unused inputs make the contract misleading and add maintenance overhead.

Prompt for AI agents
Address the following comment on .github/actions/run-vscode-e2e-tests/action.yml at line 13:

<comment>Input ssh_host is declared but not used anywhere in the action; unused inputs make the contract misleading and add maintenance overhead.</comment>

<file context>
@@ -0,0 +1,99 @@
+name: &#39;Run VS Code E2E Tests&#39;
+description: &#39;Runs VS Code E2E tests with test matrix generation and execution&#39;
+inputs:
+  github-token:
+    description: &#39;GitHub token for accessing private repositories&#39;
+    required: true
+  ci-github-token:
+    description: &#39;CI GitHub token for accessing private repositories&#39;
+    required: true
</file context>

npm run e2e:compile
if [[ "${{ inputs.is_fork }}" == "true" ]]; then
# Exclude SSH tests for forks
FILES=$(ls -1 e2e/_output/tests/*.test.js | grep -v "SSH" | jq -R . | jq -s .)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the glob e2e/_output/tests/*.test.js matches no files, ls exits with a non-zero status and the whole composite step fails even though an empty test set would be acceptable. Redirecting the error or using a safer file discovery method avoids false negatives.

Prompt for AI agents
Address the following comment on .github/actions/run-vscode-e2e-tests/action.yml at line 68:

<comment>If the glob e2e/_output/tests/*.test.js matches no files, ls exits with a non-zero status and the whole composite step fails even though an empty test set would be acceptable. Redirecting the error or using a safer file discovery method avoids false negatives.</comment>

<file context>
@@ -0,0 +1,99 @@
+name: &#39;Run VS Code E2E Tests&#39;
+description: &#39;Runs VS Code E2E tests with test matrix generation and execution&#39;
+inputs:
+  github-token:
+    description: &#39;GitHub token for accessing private repositories&#39;
+    required: true
+  ci-github-token:
+    description: &#39;CI GitHub token for accessing private repositories&#39;
+    required: true
</file context>

RomneyDa
RomneyDa previously approved these changes Aug 16, 2025
@github-project-automation github-project-automation bot moved this from Todo to In Progress in Issues and PRs Aug 16, 2025
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Aug 16, 2025
Copy link
Collaborator

@RomneyDa RomneyDa left a comment

Choose a reason for hiding this comment

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

will increase confidence in main releases

@RomneyDa RomneyDa closed this Aug 19, 2025
@RomneyDa RomneyDa reopened this Aug 19, 2025
@github-project-automation github-project-automation bot moved this from In Progress to Done in Issues and PRs Aug 19, 2025
@github-project-automation github-project-automation bot moved this from Done to In Progress in Issues and PRs Aug 19, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Aug 19, 2025
Copy link
Collaborator

@tomasz-stefaniak tomasz-stefaniak left a comment

Choose a reason for hiding this comment

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

Looks like the tests are failing to run: https://github.com/continuedev/continue/actions/runs/17064326036

@dosubot dosubot bot removed the lgtm This PR has been approved by a maintainer label Aug 19, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
size:L This PR changes 100-499 lines, ignoring generated files.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants