Skip to content

Conversation

@sobolk
Copy link
Contributor

@sobolk sobolk commented Sep 23, 2024

Problem

We have at least two tests that are using Amplify Auth to get Cognito credentials.
There's a lock guarding access to Amplify Auth apis.
However, these tests are using separate factory instances due to different configuration and effectively two separate locks.
This is wrong, because Amplify Auth apis need global locking (they're static, i.e. attached to global session).

This sometimes results in errors like this:

https://github.com/aws-amplify/amplify-backend/actions/runs/10964134931/job/30447221560

test at file:/Users/runner/work/amplify-backend/amplify-backend/packages/integration-tests/lib/test-e2e/sandbox.test.js:41:26
✖ generates config after sandbox --once deployment (22038.52175ms)
  NotAuthorizedException: Invalid session for the user.
      at <anonymous> (/Users/runner/work/amplify-backend/amplify-backend/node_modules/@aws-amplify/auth/src/providers/cognito/utils/clients/CognitoIdentityProvider/index.ts:18:19)
      at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
      at Module.confirmSignIn (/Users/runner/work/amplify-backend/amplify-backend/node_modules/@aws-amplify/auth/src/providers/cognito/apis/confirmSignIn.ts:53:138)
      at AmplifyAuthCredentialsFactory.getNewAuthenticatedUserCredentials (/Users/runner/work/amplify-backend/amplify-backend/packages/integration-tests/src/amplify_auth_credentials_factory.ts:88:7)
      at ConversationHandlerTestProject.assertPostDeployment (/Users/runner/work/amplify-backend/amplify-backend/packages/integration-tests/src/test-project-setup/conversation_handler_project.ts:165:7)
      at TestContext.<anonymous> (/Users/runner/work/amplify-backend/amplify-backend/packages/integration-tests/src/test-e2e/sandbox.test.ts:77:13)
      at async Test.run (node:internal/test_runner/test:632:9)
      at async Suite.processPendingSubtests (node:internal/test_runner/test:374:7) {
    underlyingError: undefined,
    recoverySuggestion: undefined
  }

Changes

Make lock global to match Amplify Auth api scope.

Validation

PR checks with e2e.

Checklist

  • If this PR includes a functional change to the runtime behavior of the code, I have added or updated automated test coverage for this change.
  • If this PR requires a change to the Project Architecture README, I have included that update in this PR.
  • If this PR requires a docs update, I have linked to that docs PR above.
  • If this PR modifies E2E tests, makes changes to resource provisioning, or makes SDK calls, I have run the PR checks with the run-e2e label set.

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

@sobolk sobolk added the run-e2e Label that will include e2e tests in PR checks workflow label Sep 23, 2024
@changeset-bot
Copy link

changeset-bot bot commented Sep 23, 2024

🦋 Changeset detected

Latest commit: cee9343

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 0 packages

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

check_package_versions:
if: github.event_name == 'pull_request'
runs-on: ubuntu-latest
timeout-minutes: 10
Copy link
Contributor Author

@sobolk sobolk Sep 23, 2024

Choose a reason for hiding this comment

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

Sometimes this check hangs.
This is to make time to retry faster.

@sobolk sobolk marked this pull request as ready for review September 23, 2024 16:24
@sobolk sobolk requested review from a team as code owners September 23, 2024 16:24
@sobolk sobolk merged commit c99636f into main Sep 23, 2024
58 checks passed
@sobolk sobolk deleted the global-lock branch September 23, 2024 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-e2e Label that will include e2e tests in PR checks workflow

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants