Skip to content

Adding CI to repo#32

Merged
saga-dasgupta merged 13 commits into
mainfrom
sd.run_ci
Nov 3, 2025
Merged

Adding CI to repo#32
saga-dasgupta merged 13 commits into
mainfrom
sd.run_ci

Conversation

@saga-dasgupta
Copy link
Copy Markdown
Contributor

@saga-dasgupta saga-dasgupta commented Oct 29, 2025

Broke this change into 3 commits:

  • The first commit adds CI to the repo.
  • The second commit fixes linting for this repo since the CI will run ESLint.
  • The third commit mocks all unit test calls to CLI, adds a separate step in CI for running integration test, adds test-app tests directly to the vitest config so npm test simply runs it, renames discount-function in test-app to discount-function-rs because we have cart-validation-js as the other function and finally also updates the README.

Probably easiest to review commit by commit and with whitespace changes turned off ?w=1 at the end of the url.
You can see the CI runs added and running in this PR for tests and linting.

Comment thread .gitignore
npm-debug.log*
yarn-debug.log*
yarn-error.log*
package-lock.json
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added this here so if someone accidentally uses npm instead of pnpm for building, they dont end up pushing a package-lock.json file.

Comment thread vitest.config.ts Outdated
environment: "node",
include: [
"test/**/*.test.ts",
"test-app/extensions/*/tests/**/*.test.js",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

wasm-testing-helpers-test.ts is no longer needed because the tests from test-app extensions are directly included here.

@saga-dasgupta saga-dasgupta force-pushed the sd.run_ci branch 4 times, most recently from e1cc338 to de6287e Compare October 30, 2025 15:42
@saga-dasgupta saga-dasgupta marked this pull request as ready for review October 30, 2025 15:44
Comment thread .github/workflows/ci.yml
Comment on lines +216 to +230
all-checks-pass:
name: 'All Checks Pass'
if: always()
needs: [lint-and-typecheck, test, build, integration-tests]
runs-on: ubuntu-latest
steps:
- name: Check all jobs
run: |
if [[ "${{ needs.lint-and-typecheck.result }}" != "success" ]] || \
[[ "${{ needs.test.result }}" != "success" ]] || \
[[ "${{ needs.build.result }}" != "success" ]] || \
[[ "${{ needs.integration-tests.result }}" != "success" ]]; then
echo "One or more checks failed"
exit 1
fi
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Copy Markdown
Contributor Author

@saga-dasgupta saga-dasgupta Oct 30, 2025

Choose a reason for hiding this comment

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

This acts as a single status check that waits for all jobs to complete and fails if any didn't succeed. Its for making branch protection rules easier when we have multiple parallel jobs like lint, tests (6 jobs: 3 OS × 2 Node versions), integration-tests. We can just add this one check for branch protection instead of all the 8 different checks.

Comment thread README.md Outdated
Comment thread .github/workflows/ci.yml Outdated
Copy link
Copy Markdown
Contributor

@adampetro adampetro left a comment

Choose a reason for hiding this comment

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

We should gitignore the target directory of the Rust example

Comment thread .gitignore Outdated
Copy link
Copy Markdown
Collaborator

@lopert lopert 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 target dir ended up in the PR during one of the renames, in this commit. I see Adam and you have a thread about it so I'll defer to that.

Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/ci.yml Outdated
@saga-dasgupta saga-dasgupta merged commit e07c6c6 into main Nov 3, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants