Skip to content

Conversation

@feiyangliu2023
Copy link
Contributor

Issue

D354258334

The workflow was completing successfully even when smoke tests failed because the mocha test runner returns exit code 0 regardless of test results.

Description of Changes

Fixes SageMaker smoke test workflow to properly fail when tests fail.

Capture mocha output and parse it for the "failing" keyword to detect test failures, then explicitly exit with code 1 to fail the workflow.

Testing

Verified with workflow run showing "2 failing" tests now properly fails the CI pipeline.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@feiyangliu2023 feiyangliu2023 requested a review from a team as a code owner December 5, 2025 16:00
@feiyangliu2023 feiyangliu2023 added the bug Something isn't working label Dec 5, 2025
npx playwright install
SSO_USERNAME="$SSO_USERNAME" SSO_PASSWORD="$SSO_PASSWORD" DATAZONE_URL="$DATAZONE_URL" SPACE_NAME="$SAGEMAKER_SPACE_NAME" VSCODE_REMOTE_SERVER_PATH="sagemaker" npm run mocha -- --web --headless

SSO_USERNAME="$SSO_USERNAME" SSO_PASSWORD="$SSO_PASSWORD" DATAZONE_URL="$DATAZONE_URL" SPACE_NAME="$SAGEMAKER_SPACE_NAME" VSCODE_REMOTE_SERVER_PATH="sagemaker" npm run mocha -- --web --headless 2>&1 | tee test_output.log
Copy link
Contributor

@vpaiu vpaiu Dec 8, 2025

Choose a reason for hiding this comment

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

For now, I think this fix is good. But we should later look into why npm run mocha doesn't exit with code 1 on tests failing.

A think a fix would be using the actual smoketest npm command (https://github.com/microsoft/vscode/blob/main/package.json#L43). As when I was working on the smoke testing I didn't notice it in the package.json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, will look into it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

smoketest command will not be used as it rebuilds entire vscode src which makes space out of scope

@vpaiu
Copy link
Contributor

vpaiu commented Dec 8, 2025

Another improvement we can make: I noticed when we construct the DATAZONE_URL we hardcode the region, but we should instead use the AWS_REGION env variable.

@feiyangliu2023 feiyangliu2023 had a problem deploying to sagemaker-e2e-tests-workflow-env December 8, 2025 10:58 — with GitHub Actions Failure

- name: Download sagemaker build artifact
run: |
gh run download --name "$COMMIT_SHA-code-editor-sagemaker-server-build"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should still have the commit sha

- name: Fetch source artifact
run: |
gh run download --name "$COMMIT_SHA-code-editor-sagemaker-server-src"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should still have the commit sha

@feiyangliu2023 feiyangliu2023 had a problem deploying to sagemaker-e2e-tests-workflow-env December 8, 2025 11:03 — with GitHub Actions Failure
@feiyangliu2023 feiyangliu2023 had a problem deploying to sagemaker-e2e-tests-workflow-env December 8, 2025 12:32 — with GitHub Actions Failure
@feiyangliu2023 feiyangliu2023 had a problem deploying to sagemaker-e2e-tests-workflow-env December 8, 2025 13:05 — with GitHub Actions Failure
@feiyangliu2023 feiyangliu2023 had a problem deploying to sagemaker-e2e-tests-workflow-env December 8, 2025 13:29 — with GitHub Actions Failure
@feiyangliu2023 feiyangliu2023 had a problem deploying to sagemaker-e2e-tests-workflow-env December 8, 2025 13:56 — with GitHub Actions Failure
@feiyangliu2023 feiyangliu2023 had a problem deploying to sagemaker-e2e-tests-workflow-env December 8, 2025 14:23 — with GitHub Actions Failure
@vpaiu vpaiu merged commit f94909a into main Dec 8, 2025
3 checks passed
@vpaiu vpaiu deleted the sagemaker-e2e-test branch December 8, 2025 15:03
feiyangliu2023 added a commit to feiyangliu2023/code-editorv2 that referenced this pull request Dec 10, 2025
Co-authored-by: Feiyang Liu <lfeiyang@amazon.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants