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

Fix e2e tests for pluralization #7590

Merged
merged 3 commits into from
Jun 24, 2021
Merged

Fix e2e tests for pluralization #7590

merged 3 commits into from
Jun 24, 2021

Conversation

johnpc
Copy link
Contributor

@johnpc johnpc commented Jun 23, 2021

Description of changes

When #7258 was merged, e2e tests began failing because of the new improvePluralization feature flag behavior during certain test runs.

We test the iterative update feature by renaming the intitial Todo model and updating it to Todos. Previously (before #7258), this created a resolver called listTodoss (because it incorrectly blindly appended s). With the pluralization fix, it creates a resolver called listTodos - which unfortunately already exists and therefore appsync throws a Only one resolver is allowed per field error (see appsync bug).

This PR fixes the broken e2e tests by renaming the model from Todo to Task instead of to Todos, therefore eliminating the conflict.

Issue #, if available

#4224

Description of how you validated changes

  • ran e2e tests

Checklist

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

@johnpc johnpc requested a review from a team as a code owner June 23, 2021 16:31
@cjihrig
Copy link
Contributor

cjihrig commented Jun 23, 2021

I would prefer to land #7584 first so that the unrelated typo fix is not at risk of being reverted again.

Currently, we’re testing the iterative update by renaming the intial Todo model and updating it to Todos. Previously, this created a resolver called listTodoss (because it incorrectly blindly appended s). With the pluralization fix, it creates a resolver called listTodos - which unfortunately already exists and therefore appsync throws a Only one resolver is allowed per field error (see appsync bug)

ref:

appsync bug: aws/aws-appsync-community#128

initial todo model: https://github.com/aws-amplify/amplify-cli/blob/master/packages/amplify-e2e-tests/schemas/iterative-push/change-model-name/initial-schema.graphql#L6
final todo model: https://github.com/aws-amplify/amplify-cli/blob/master/packages/amplify-e2e-tests/schemas/iterative-push/change-model-name/final-schema.graphql#L6
@edwardfoyle edwardfoyle merged commit 1b01540 into aws-amplify:master Jun 24, 2021
cjihrig pushed a commit to ctjlewis/amplify-cli that referenced this pull request Jul 12, 2021
Currently, we’re testing the iterative update by renaming the intial Todo model and updating it to Todos. Previously, this created a resolver called listTodoss (because it incorrectly blindly appended s). With the pluralization fix, it creates a resolver called listTodos - which unfortunately already exists and therefore appsync throws a Only one resolver is allowed per field error (aws/aws-appsync-community#128)
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