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: e2e tests on windows #750

Merged
merged 14 commits into from
Oct 31, 2023
Merged

ci: e2e tests on windows #750

merged 14 commits into from
Oct 31, 2023

Conversation

dpilch
Copy link
Member

@dpilch dpilch commented Oct 23, 2023

Description of changes

Run E2E tests on Windows.

The Windows bash terminal on CodeBuild appears to be non-interactive. This is causing issues with selecting a region when running amplify configure. I've set all the Windows E2E tests to run us-east-1 for now and we can fix that later when you have some more time.

There are two disabled tests for windows only:

  1. amplify pull on an ios project.
  2. Cypress run after generating graphql statements.

Test 1 is not very valuable. It is possible to do this with the current CLI, but realistically no customer would do this because XCode is not available on windows.

Test 2 is much more valuable. This asserts that the generated graphql statements work in a running app. The app compiles but then there is an error in cypress. It does not appear to be an error in a test case. It might be an issue with cypress on the windows instance.

I was not able to reproduce either case on my own windows workspace.

To recap, we have 3 follow ups to complete later:

  1. Windows E2E tests on other regions
  2. Enable amplify pull on ios project test
  3. Enable Cypress run after generating graphql statements test.

Issue #, if available

N/A

Description of how you validated changes

Run the E2E tests.

Checklist

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@dpilch dpilch force-pushed the windows-e2e branch 2 times, most recently from b152483 to ffcedcb Compare October 25, 2023 17:55
set -e

source ./shared-scripts.sh && _setupE2ETestsWindows
codebuild-breakpoint
Copy link
Member Author

Choose a reason for hiding this comment

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

This is keeping the same behavior as E2E tests on linux. I'm not sure codebuild-breakpoint is strictly needed on either.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is used when you want to run the tests in debug mode. I think the script is called yarn-e2e-debug.

.wait(
'"amplify publish" will build all your local backend and frontend resources (if you have hosting category added) and provision it in the cloud',
)
.wait('"amplify publish" will build all your local backend and frontend resources')
Copy link
Member Author

Choose a reason for hiding this comment

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

Strange error.

● codegen remove tests - iOS › Does not delete files during codegen remove
--
351 |  
352 | Expected value   "\"amplify publish\" will build all your local backend and frontend resources (if you have hosting category added) and provision it in the cloud"
353 | Received:
354 | "ision it in the cloud"
355 |  
356 | Message:
357 | expected "ision it in the cloud" to contain "\"amplify publish\" will build all your local backend and frontend resources (if you have hosting category added) and provision it in the cloud"
358 |  
359 | Difference:
360 |  
361 | - Expected
362 | + Received
363 |  
364 | - "amplify publish" will build all your local backend and frontend resources (if you have hosting category added) and provision it in the cloud
365 | + ision it in the cloud

The message is correct when running manually. Maybe there is a maximum length? The "v" in "provision" is the 120th character.

This change was also made on the CLI E2E when enabling windows.
aws-amplify/amplify-cli@97c6de4

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, you can wrap the message with */ regex to ignore subsequent parts. Just FYI.

function _scanArtifacts {
if ! yarn ts-node .codebuild/scripts/scan_artifacts.ts; then
Copy link
Member Author

Choose a reason for hiding this comment

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

Get an error when using yarn over npx/npm. It looks like there are some workarounds, but it is easier to just use npx/npm here because there will be no functional difference to the scripts.

error Couldn't find the binary C:\codebuild\tmp\output\src2004\src\github.com\aws-amplify\amplify-codegen\node_modules\.bin\ts-node

@@ -7,7 +7,7 @@ function startLocalRegistry {
# Start local registry
tmp_registry_log="$(mktemp)"
echo "Registry output file: $tmp_registry_log"
(cd && nohup npx ${VERDACCIO_PACKAGE:-$default_verdaccio_package} -c $1 &>$tmp_registry_log &)
nohup npx ${VERDACCIO_PACKAGE:-$default_verdaccio_package} -c $1 &>$tmp_registry_log &
Copy link
Member Author

Choose a reason for hiding this comment

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

The cd is unnecessary here and caused some issues on Windows.

@@ -32,7 +32,7 @@
"jest-circus": "^27.5.1",
"jest-environment-node": "^26.6.2",
"lodash": "^4.17.19",
"node-pty": "beta",
"node-pty": "1.0.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

This might not be necessary for this change. I tried this when troubleshooting various issues. However, this is probably something we should do regardless so that our version is stable. I can try removing this and adding it to a separate change if it is desired.

@dpilch dpilch marked this pull request as ready for review October 26, 2023 21:21
@dpilch dpilch requested review from a team as code owners October 26, 2023 21:21
@dpilch dpilch marked this pull request as draft October 26, 2023 21:22
@dpilch dpilch marked this pull request as ready for review October 30, 2023 18:45
// amplify-configure will always choose us-east-1 for the region.
// This will set all Windows jobs to use us-east-1 to avoid region mismatch.
// We will come back to this later to properly fix the testing issue.
const region = os === 'w' ? 'us-east-1' : AWS_REGIONS_TO_RUN_TESTS[jobIdx % AWS_REGIONS_TO_RUN_TESTS.length];
Copy link
Member Author

@dpilch dpilch Oct 30, 2023

Choose a reason for hiding this comment

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

Another weird thing, this only seems to affect the amplify configure step. On commands like codegen add the selection is set correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

We might need to look into how we're sending the region selection command interactively and make it for both platforms. Running in us-east-1 is fine for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is using j/k to navigate the list. I tried with up/down arrow as well and ran into the same issue. I've created a task to investigate further.

set -e

source ./shared-scripts.sh && _setupE2ETestsWindows
codebuild-breakpoint
Copy link
Contributor

Choose a reason for hiding this comment

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

This is used when you want to run the tests in debug mode. I think the script is called yarn-e2e-debug.

.wait(
'"amplify publish" will build all your local backend and frontend resources (if you have hosting category added) and provision it in the cloud',
)
.wait('"amplify publish" will build all your local backend and frontend resources')
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, you can wrap the message with */ regex to ignore subsequent parts. Just FYI.

@@ -31,9 +32,10 @@ describe('GraphQL documents generator e2e tests', () => {
});

const schemaFileName = 'schema.graphql';
it('generates valid GraphQL documents for given schema', async () => {
// skip cypress test on windows
(isWindows() ? it.skip : it)('generates valid GraphQL documents for given schema', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to run these on windows as well eventually? If yes, we should have a task for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've created tasks for the 2 disabled tests and the region issue.

// amplify-configure will always choose us-east-1 for the region.
// This will set all Windows jobs to use us-east-1 to avoid region mismatch.
// We will come back to this later to properly fix the testing issue.
const region = os === 'w' ? 'us-east-1' : AWS_REGIONS_TO_RUN_TESTS[jobIdx % AWS_REGIONS_TO_RUN_TESTS.length];
Copy link
Contributor

Choose a reason for hiding this comment

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

We might need to look into how we're sending the region selection command interactively and make it for both platforms. Running in us-east-1 is fine for now.

@dpilch dpilch merged commit 8ec490d into main Oct 31, 2023
5 checks passed
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.

3 participants