-
Notifications
You must be signed in to change notification settings - Fork 0
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
ci: setup e2e tests workflow and local webpack e2e test #508
base: main
Are you sure you want to change the base?
Conversation
- name: Setup node and build the repository | ||
uses: ./amplify-data/.github/actions/node-and-build | ||
- name: Make script executable | ||
run: chmod +x ./amplify-data/scripts/retry-npm-script.sh |
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.
Do we chmod scripts like this in other places? I thought the file permissions were checked in.
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.
No, I had to add this step to avoid error like this. I thought the same way too, and I wonder if I missed some steps here since it should not be needed.
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.
Resolved. Changing the file permissions locally and pushing the change made it work.
scripts/test.mjs
Outdated
const FRAMEWORKS = { | ||
rollup: 'rollup', | ||
webpack: 'webpack', | ||
}; | ||
|
||
const BUILD_TYPE = { | ||
dev: 'dev', | ||
prod: 'prod', | ||
}; | ||
|
||
const frameworkPort = { | ||
[FRAMEWORKS.rollup]: '3000', | ||
[FRAMEWORKS.webpack]: '3000', | ||
}; |
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.
It looks like we're using this script to fan out.
If these are long running, should we fan them out in workflows on separate instances?
If they are not long running, do we need to have them running concurrently? Wondering if we can simplify the logic to have the run synchronously if we need them running on the same test runner.
Mostly, there's a lot of logic here and I'm wondering if there's a different pattern to achieve the same or a similar outcome with less complexity.
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 script is called by the workflow here. The logic is mainly used to validate the inputs of the test and set up port numbers for it. At any time, only 1 framework with 1 build type is being tested. The concurrency is only used to set up the amplify app sample for a given test config, and run the test against it. This file is modified from the original test.js file in samples-staging
repo, and it might be an overkill if we don't plan to add more complicated e2e tests in the future.
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 have rewritten the test in a shell script. It only contains basic logic for setting up a server and testing against it. We can expand upon it in the future if we decide to bring in other tests.
package.json
Outdated
@@ -77,11 +80,13 @@ | |||
"jsdom": "^25.0.0", | |||
"prettier": "^3.0.3", | |||
"rimraf": "^5.0.5", | |||
"serve": "^14.2.4", |
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 push this dependency down into the test repo and run a command from there to launch it instead of having this dep in the package root?
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 pushed it down to the webpack
folder, and created a command there for serve
.
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.
Change looks good and in the example run is passing. Do we know why other e2es are failing in that run?
Yes, the local node test sometimes fails and re-running the workflow will fix it. This happened in the past as well. The sandbox e2e test failed due to an authorization error, which won't happen in the main branch. |
Issue #, if available:
Description of changes:
Example pipeline run
This change is a part of the E2E test roll out plan for
amplify-data
repository. It can be summarized into two parts:webpack
bundler support.amplify-data
repository.For the test, this change uses webpack to bundle a default Amplify app into a
javascript
file:main.bundle.js
.Amplify app:
Test Process in Git workflows:
index.html
file.localhost:3000
.Any other status code will result in a test failure.
This change also adds the necessary GitHub actions config, scripts, and testing dependencies. Once this PR is merged, tests will run in the pipeline.
This change also modifies the current documentation for E2E tests, providing detailed instructions for local e2e testing.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.