Skip to content
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

Open
wants to merge 77 commits into
base: main
Choose a base branch
from

Conversation

zxl629
Copy link
Contributor

@zxl629 zxl629 commented Jan 22, 2025

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:

  1. Adds a sample app to test webpack bundler support.
  2. Adds webpack E2E test as a release blocker for 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:

import { generateClient } from 'aws-amplify/data';
import { Schema } from '@/amplify/data/resource';

const client = generateClient<Schema>();

Test Process in Git workflows:

  1. File Preparation: The bundled JavaScript file will be incorporated as the source script in the index.html file.
  2. Server Setup: GitHub runners will build the project and start a local development server. The server will be configured to listen on localhost:3000.
  3. Request Execution: The test will use the curl CLI tool to send a GET request to http://localhost:3000.
  4. Response Evaluation: A successful test will receive an HTTP status code 200, indicating that the page loaded correctly.
    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.

- 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
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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
Comment on lines 10 to 23
const FRAMEWORKS = {
rollup: 'rollup',
webpack: 'webpack',
};

const BUILD_TYPE = {
dev: 'dev',
prod: 'prod',
};

const frameworkPort = {
[FRAMEWORKS.rollup]: '3000',
[FRAMEWORKS.webpack]: '3000',
};
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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",
Copy link
Member

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?

Copy link
Contributor Author

@zxl629 zxl629 Jan 24, 2025

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.

@zxl629 zxl629 requested a review from stocaaro January 24, 2025 18:20
Copy link
Member

@stocaaro stocaaro left a 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?

@zxl629
Copy link
Contributor Author

zxl629 commented Jan 31, 2025

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.

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