test(amplify-e2e-tests): add tests for graphql schemas in doc

[UnleashedMind]

Add tests for graphql schemas in the document Amplify CLI/API(GraphQL)/Directives

Issue #, if available:

Description of changes:

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

[yuth]

The main goal with tests should be verification (which this change does) and readability of the test code so one knows what is going on (and make it easier to figure out why tests are failing) rather then reducing code duplication.

Could you make the schema, query and mutation inline and use the runQuery, runMutation in the test itself. Also use the proper asserting for tests instead of returning true and expecting tests to return true as it makes it harder to figure out why a test failed

[codecov]

Codecov Report

Merging #4092 into master will increase coverage by 0.42%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4092      +/-   ##
==========================================
+ Coverage   60.51%   60.93%   +0.42%     
==========================================
  Files         350      347       -3     
  Lines       15199    15099     -100     
  Branches     2878     2860      -18     
==========================================
+ Hits         9197     9200       +3     
+ Misses       5514     5411     -103     
  Partials      488      488              

Impacted Files	Coverage Δ
...es/graphql-connection-transformer/src/resources.ts	78.20% <ø> (ø)
...tion-transformer/src/ModelConnectionTransformer.ts	89.52% <100.00%> (+0.10%)	⬆️
packages/amplify-util-mock/src/func/index.ts
packages/amplify-util-mock/src/api/index.ts
...ages/amplify-util-mock/src/utils/lambda/execute.ts

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 827c7b8...e7a4524. Read the comment docs.

[lgtm-com]

This pull request introduces 3 alerts when merging f0cf6970bb23a3f40943fa7938d2e0e9dc118a74 into 15eac84 - view on LGTM.com

new alerts:

• 2 for Unused variable, import, function or class
• 1 for Duplicate character in character class

[lgtm-com]

This pull request introduces 3 alerts when merging 0ff23d47f2f58a07ebfad4d7677e3f38619f2b61 into 15eac84 - view on LGTM.com

new alerts:

• 2 for Unused variable, import, function or class
• 1 for Duplicate character in character class

[lgtm-com]

This pull request introduces 3 alerts when merging 23a9907 into 709491f - view on LGTM.com

new alerts:

• 2 for Unused variable, import, function or class
• 1 for Duplicate character in character class

[UnleashedMind]

@yuth

In response to the following comments:

"The main goal with tests should be verification (which this change does) and readability of the test code so one knows what is going on (and make it easier to figure out why tests are failing) rather then reducing code duplication.

Could you make the schema, query and mutation inline and use the runQuery, runMutation in the test itself. Also use the proper asserting for tests instead of returning true and expecting tests to return true as it makes it harder to figure out why a test failed"

The schema being tested, the mutations, queries and subscriptions to test the schema, and their expected responses are consolidated into one file for better readability.
If the test for a schema does not fall into one of the common test patterns, the test method itself (runTest) is also in the same file as the schema.

The test compares the actual mutation, query and subscription responses with the expected results. As the responses can be objects of depth, the test goes to a max depth of 50 (configurable constant) trying to identify mismatches. I can't figure out a simple assertion for that. But if there is a mismatch, the cause of mismatch is clearly printed out, so there shouldn't be any problem identifying the failed test, and the part that failed it.

[yuth]

The schema being tested, the mutations, queries and subscriptions to test the schema, and their expected responses are consolidated into one file for better readability.
If the test for a schema does not fall into one of the common test patterns, the test method itself (runTest) is also in the same file as the schema.

Consolidating them to one file is counter productive. The most important thing with tests is

1. Testing of functionality
2. Readability as tests are living documentation of behaviors.

We have similar tests in our GraphQL E2E tests. In those tests, the schema and mutation is all contained in the same file and makes it easy to understand what is going on.

Schema

amplify-cli/packages/graphql-transformers-e2e-tests/src/__tests__/DynamoDBModelTransformer.e2e.test.ts

Lines 39 to 77 in 8e0662e

	beforeAll(async () => {
	const validSchema = /* GraphQL */ `
	type Post @model {
	id: ID!
	title: String!
	createdAt: AWSDateTime
	updatedAt: AWSDateTime
	metadata: PostMetadata
	entityMetadata: EntityMetadata
	appearsIn: [Episode!]
	episode: Episode
	}
	type Author @model {
	id: ID!
	name: String!
	postMetadata: PostMetadata
	entityMetadata: EntityMetadata
	}
	type EntityMetadata {
	isActive: Boolean
	}
	type PostMetadata {
	tags: Tag
	}
	type Tag {
	published: Boolean
	metadata: PostMetadata
	}
	enum Episode {
	NEWHOPE
	EMPIRE
	JEDI
	}
	type Comment @model(timestamps: { createdAt: "createdOn", updatedAt: "updatedOn" }) {
	id: ID!
	title: String!
	content: String
	updatedOn: Int # No automatic generation of timestamp if its not AWSDateTime
	}

Test

amplify-cli/packages/graphql-transformers-e2e-tests/src/__tests__/DynamoDBModelTransformer.e2e.test.ts

Lines 189 to 224 in 8e0662e

	test('Test createAuthor mutation', async () => {
	try {
	const response = await GRAPHQL_CLIENT.query(
	`mutation($input: CreateAuthorInput!) {
	createAuthor(input: $input) {
	id
	name
	entityMetadata {
	isActive
	}
	createdAt
	updatedAt
	}
	}`,
	{
	input: {
	name: 'Jeff B',
	entityMetadata: {
	isActive: true,
	},
	},
	},
	);
	expect(response.data.createAuthor.id).toBeDefined();
	expect(response.data.createAuthor.name).toEqual('Jeff B');
	expect(response.data.createAuthor.createdAt).toBeDefined();
	expect(response.data.createAuthor.updatedAt).toBeDefined();
	expect(response.data.createAuthor.entityMetadata).toBeDefined();
	expect(response.data.createAuthor.entityMetadata.isActive).toEqual(true);
	} catch (e) {
	console.log(e);
	// fail
	expect(e).toBeUndefined();
	}
	});

The test compares the actual mutation, query and subscription responses with the expected results. As the responses can be objects of depth, the test goes to a max depth of 50 (configurable constant) trying to identify mismatches. I can't figure out a simple assertion for that. But if there is a mismatch, the cause of mismatch is clearly printed out, so there shouldn't be any problem identifying the failed test, and the part that failed it.

I have added an example test in #4092 (comment)

Could you explain what you mean by max depth of 50? Jest has the assertion expect(result).toEqual({}) which does compare objects. The other option is to use expect(result).toMatchInlineSnapshot() which keeps the exact result.

[yuth]

Can we use .toMatchObject(object) instead of rolling our own solution

[UnleashedMind]

There are some technical difficulties.
In the test, the actual received mutation, query and subscription results are not directly compared with the expected results specified in the test case file (e.g. using the jest .toMatchObject(object) method).

• id, createdAt and updatedAt fields are generated (if not specified in the mutation), their values are not determined when the test starts to run. Those fields are generally given a value “” in the expected result specification, and the test only checks that they are defined in the actual received result.
• If error is expected, the test only checks for the error type, but the actual error response contains a lot of fields.
• The actual response can contain the "__typename" field, even if the schema does not contain union of interface, and even if the mutation or query does not specify it. The expected result in the test case files do not include this extra field.

To compare the expected and the actual result object, the test uses a BFS method to find mismatched field, if no mismatch is found after MAX_DEPTH (set at 50 in the test), the test passes.

[yuth]

Please dont use the variable naming magic. Like i suggested in an earlier comment, move this inside a test and then wrap it inside an object, so both mutation and results are wrapped in the same object

[UnleashedMind]

This is related to the test layout that we discussed in the meeting.

[lgtm-com]

This pull request introduces 1 alert when merging 1cbee0c into 827c7b8 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[lgtm-com]

This pull request introduces 1 alert when merging 8b0521f into 0a6a7ea - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[lgtm-com]

This pull request introduces 2 alerts when merging 12f493d into 0a6a7ea - view on LGTM.com

new alerts:

• 2 for Unused variable, import, function or class

[lgtm-com]

This pull request introduces 2 alerts when merging e7a4524 into 454fdc0 - view on LGTM.com

new alerts:

• 2 for Unused variable, import, function or class

[github-actions]

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels for those types of questions.