-
Notifications
You must be signed in to change notification settings - Fork 2
Integration test overhaul #27
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
Conversation
142ee12 to
4f817ad
Compare
| // const harperAppLock = await readFile(join(ctx.harper.installDir, 'harper-application-lock.json'), 'utf-8'); | ||
| // deepStrictEqual(JSON.parse(harperAppLock), { | ||
| // applications: { | ||
| // 'test-application': {} | ||
| // } | ||
| // }); |
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 need to cherry-pick the recent application lock fix to this repo and then I can uncomment this assertion.
kriszyp
left a comment
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.
This looks like it will be awesome, excited to see more tests brought in!
|
Generally this is pretty awesome and most of the feedback I've thought of would be non-blocking edge case handling and 'nice to haves' (e.g. loopback cleanup - i.e. leave no trace). This (test utils especially) might be nice for others to use in their own Harper apps and components/plugins. Have you thought about publishing as a separate package? |
cb1kenobi
left a comment
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.
This all looks really awesome! Great job!
| const parts = address.split('.'); | ||
| if (parts.length !== 4 || parts[0] !== '127' || parts[1] !== '0' || parts[2] !== '0') { | ||
| throw new InvalidLoopbackAddressError(address); | ||
| } | ||
|
|
||
| const index = parseInt(parts[3], 10) - 1; |
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.
You could use a regex instead of splitting.
| const parts = address.split('.'); | |
| if (parts.length !== 4 || parts[0] !== '127' || parts[1] !== '0' || parts[2] !== '0') { | |
| throw new InvalidLoopbackAddressError(address); | |
| } | |
| const index = parseInt(parts[3], 10) - 1; | |
| const m = address.match(/^127\.0\.0\.(\d{1,3})$/); | |
| if (!m?.[1]) { | |
| throw new InvalidLoopbackAddressError(address); | |
| } | |
| const index = Number.parseInt(m[1], 10) - 1; |
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.
Personally, I'm not a huge fan of using regex due to its readability. I understand it can be better performance but since this is just a util script I don't think I'm going to make this change.
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.
Heh, I was thinking the regex was so simple that it would be more readable than the split version.
96ced60 to
71bea0e
Compare
|
#51 will fix the failing format check |
kriszyp
left a comment
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.
Great work, let's start using it!
71bea0e to
f5af665
Compare
- New integration test guidelines and framework - Updated GitHub Actions workflow - Continue to support existing integration tests - Copious documentation and utility scripts
f5af665 to
3a3e0eb
Compare
Integration Test Overhaul
This PR introduces a comprehensive overhaul of Harper's integration testing infrastructure, enabling concurrent test execution for faster feedback cycles and improved developer productivity.
Overview of Changes
New Integration Test Framework:
setupHarper,teardownHarper) with automatic resource cleanupDocumentation:
integrationTests/README.mdcovering test writing guidelines, execution strategies, and best practicesintegrationTests/utils/README.mdCONTRIBUTING.mdwith new integration test setup requirementsCI/CD Improvements:
New Test Infrastructure:
Example Tests:
deploy-from-source.test.mts- Deploy application from compressed sourcedeploy-from-github.test.mts- Deploy application from GitHub repositoryAdditional Changes:
How to Review
Start with the documentation - Read
integrationTests/README.mdto understand the new testing philosophy, requirements, and execution strategies. This provides critical context for all other changes.Review the utilities - Check
integrationTests/utils/README.mdand the utility implementations inintegrationTests/utils/to understand the building blocks available for writing tests.Examine the test examples - Look at the deploy tests (
deploy-from-source.test.mtsanddeploy-from-github.test.mts) as reference implementations showing how to use the new framework and utilities.Review CI/CD changes - Check
.github/workflows/integration-tests.ymlto understand how tests are parallelized across runners and how sharding works.Check supporting changes - Review smaller changes to source files (installer, logging, etc.) that support better test isolation and execution.
Testing
The new integration tests require loopback address configuration. Run the setup script if needed:
Then execute tests: