Skip to content

enable running the sdk-tests locally #109

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

Merged
merged 2 commits into from
Jun 17, 2022
Merged

Conversation

JakeChampion
Copy link
Contributor

I wanted to get the tests working locally and I couldn't get the act to work correctly, which is what run-local-integration-tests.sh relied on.

I thought we could perhaps make it so that the sdk-tests github action does not rely on anything github action specific and then we can run the action's script ourselves via node .github/actions/compute-sdk-test/main.js.

To do this I've chosen to hard-code the test configuration path as we only have one configuration file, this removed the need for the github action specific @actions/core npm package. We also had in the code the @actions/github package but were never using it, so that has also been removed.

The final change needed to make running the tests locally was to update the npm run build:test command to look for the js-compute-runtime cli within ./target/release/.

To run the integration tests locally we can now do this:
cargo build --release && node .github/actions/compute-sdk-test/main.js

Note: During the combining of the different repositories we brought in a git pre-commit hook which is running a code-formatter on any modified files, this is why the commits contain lots of format changes.

Comment on lines 5 to 7
const fetch = require("node-fetch");
const chalk = require("chalk");
require("better-logging")(console, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we can replace these dependencies with nodejs built-in packages such as http, then we will no longer need to have a build step nor need to commit the dist directory as we could run this file directly with GitHub Actions

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm all for it!

@JakeChampion JakeChampion force-pushed the jake/local-integration-tests branch from 66027e7 to f768827 Compare June 16, 2022 11:15
@JakeChampion JakeChampion marked this pull request as ready for review June 16, 2022 12:53
Copy link
Contributor

@elliottt elliottt left a comment

Choose a reason for hiding this comment

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

This looks great to me, thanks!

Re the formatting pre-commit hook: let's remove the npm dependencies that set that up before merging this pr, so that we can minimize the diff.

…tion specific functions in the sdk-test script

I've chosen to hard-code the test configuration path as we only have one configuration file

To run the integration tests locally we can now do this:
`cargo build --release && node .github/actions/compute-sdk-test/main.js`
@JakeChampion JakeChampion force-pushed the jake/local-integration-tests branch from f768827 to 743793f Compare June 17, 2022 09:36
@JakeChampion JakeChampion force-pushed the jake/local-integration-tests branch from 743793f to d59c180 Compare June 17, 2022 09:38
@JakeChampion JakeChampion merged commit ade5133 into main Jun 17, 2022
@JakeChampion JakeChampion deleted the jake/local-integration-tests branch June 17, 2022 09:49
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.

2 participants