- Notifications
You must be signed in to change notification settings - Fork72
chore: update CONTRIBUTION docs to explain both tests, and update CI for both tests#384
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
This PR updates the documentation and CI pipeline to clarify that the repository supports both TypeScript and Terraform testing approaches. The changes align the documentation with the actual testing practices and ensure both test types run in CI.
- Update CONTRIBUTING.md to explain both TypeScript tests (
bun test) and Terraform tests (terraform test) - Add Terraform test execution to the CI workflow
- Clarify testing requirements for new vs existing modules
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| CONTRIBUTING.md | Updated documentation to explain dual testing approaches and provide clear guidance for both TypeScript and Terraform tests |
| .github/workflows/ci.yaml | Added Terraform test execution step to ensure both test types run in CI |
Tip: Customize your code reviews with copilot-instructions.md.Create the file orlearn how to get started.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Atif Ali <atif@coder.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
left a few suggestions. I think we need to have both simultaneously.
TypeScript tests can be skipped if the module does not run any scripts.
Ahh I okay I'm seeing the whole picture better now. I'll commit your other change and give it one more look over before it's merged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we update the script inpackage.json to run all tests when someone runbun test
Uh oh!
There was an error while loading.Please reload this page.
Doesn't the package.json currently do this already? |
I think we just need to omit the examples from the terraform_test_all.sh script. I will do this tomorrow morning and get this finished up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Current changes look good! Just wasn't sure if there was some extra context we could add
Uh oh!
There was an error while loading.Please reload this page.
@matifali@Parkreiner If you can approve this again I resolved the failing tf tests so now this is good for all new/existing PR's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good to me. Just had one small nit
| type=string | ||
| description="The path to logMODULE_NAME to." | ||
| default="/tmp/MODULE_NAME.log" | ||
| description="The path to logmodule-name to." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think this is a huge deal, but I could see it being a slight source of confusion: is there a reason why we usemodule-name for the descriptions, but still usemodule_name for the resource names?
Closes#383
Description
Type of Change
Testing & Validation
bun test)bun run fmt)