chore: enable eslint for tests for adapter-nextjs aws-amplify and core #13108

HuiSF · 2024-03-11T22:13:35Z

Description of changes

Details please see each commit of this PR. Recommend to focus on the commits that contain manual fixes.

Issue #, if available

Description of how you validated changes

Checklist

PR description included
yarn test passes
Tests are changed or added
Relevant documentation is changed or added (and PR referenced)

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

svidgen

I am curious for others' opinions on the promise param naming rules that seem to be on by default. I commented on one example, but there are a few others.

The rule potentially hurts readability by preventing the use of more semantically meaningful and/or "scoped" names in favor of the vague/generic naming. In some cases, I seem to recall renaming promise params to visually and mentally disentangle them and clearly communicate intent when promises are nested and/or "near" each other.

I don't want to block on this. But, if others feel the same, I would support disabling that rule.

svidgen · 2024-03-12T22:04:19Z

packages/core/__tests__/BackgroundProcessManager.test.ts

-		manager.add(async () => new Promise(unsleep => setTimeout(unsleep, 10)));
+		manager.add(async () => new Promise(resolve => setTimeout(resolve, 10)));


Strange. This one actually kind of bugs me. I feel like this rule has the potential to hinder readability.

But, I'm curious for others' opinions on this.

Lol i was going to comment. the rule seems to not have any sense of humor

Yea, I'm not holding on this strongly, but I can see the downside of renaming the executor functions for a type of use case. For example, in some places, e.g., in api-graphql and pubsub packages, we have quite a few promise executor implementations have a number of lines more than one page, and a sudden appearance of unblock, unsleep, etc. may actually hurt the readability which resolve and reject are the default names, which are related to a promise naturally.

For readability around promise... I think can also be achieved by better variable names, or creating a meaningful wrapper. For this particular case for example

const sleep = (seconds: number) => new Promise(resolve => setTimeout(resolve, seconds)); manager.add(sleep(10));

should provide even better readability. Wdyt?

Just to add my 2 cents here. I think I would find it more confusing that a promise has anything other than resolve /reject than it would help to try and contextualize them in place. With resolve - I understand immediately what it is doing within a promise body (especially a long one, as @HuiSF pointed out), whereas with unsleep I have to understand ahead of time that that it's resolving the promise and not some magic function defined elsewhere.

Agree. I sometimes tend to name something like in ResolvesWith or ResolvesSomething along those lines. I was more curious along those lines. Since a strict resolve/reject might enforce that right?

It's actually the lengthier promises that benefit from a semantically named resolver, IMHO. But, I won't stomp my feet for this unless it bites me. Perhaps it'll be a good forcing function to refactor and reduce inline nesting.

packages/core/__tests__/BackgroundProcessManager.test.ts

ashika112 · 2024-03-15T00:08:12Z

packages/core/__tests__/Hub.test.ts

@@ -67,7 +73,9 @@ describe('Hub', () => {
 		);
 	});
 	test('Remove listener', () => {
-		const listener = jest.fn(() => {});
+		const listener = jest.fn(() => {
+			// no-op


I dont think this rule would work for tests. This seems affect readability honestly. I dont see value with this.

packages/core/__tests__/utils/queuedStorage/queuedStorage.native.test.ts

packages/core/__tests__/clients/composeApiHandler.test.ts

ashika112 · 2024-03-15T00:14:24Z

@HuiSF Do u think for __tests__ maybe remove some rules that wouldn't make sense on it?

HuiSF · 2024-03-15T17:40:04Z

@HuiSF Do u think for __tests__ maybe remove some rules that wouldn't make sense on it?

My own answer would be no to this question, we should treat __tests__ codebase with the same standard to ensure the same quality. I believe this helps us writing robust test suites. Open for further discussion.

packages/adapter-nextjs/__tests__/api/generateServerClient.test.ts

cshfang · 2024-03-19T17:43:38Z

packages/core/__tests__/utils/queuedStorage/queuedStorage.test.ts

+					return () => {
+						// no op
+					};


Nit: Not sure but is it possible to use jest.fn for some of these as well?

Using jest.fn() as the return type Jest.Mock would change the parameter type on the setter function. As setter and getter are always a pair...

packages/core/__tests__/utils.test.ts

packages/core/__tests__/storage/CookieStorage.test.ts

packages/core/__tests__/storage/DefaultStorage.test.ts

packages/core/__tests__/Platform/customUserAgent.test.ts

packages/core/__tests__/Hub.test.ts

cshfang · 2024-03-19T18:16:22Z

packages/core/__tests__/ConsoleLogger.test.ts

+		jest.spyOn(console, 'log').mockImplementation(noop);
+		jest.spyOn(console, 'error').mockImplementation(noop);
+		jest.spyOn(console, 'warn').mockImplementation(noop);
+		jest.spyOn(console, 'info').mockImplementation(noop);
+		jest.spyOn(console, 'debug').mockImplementation(noop);


I wonder if these even need their implementations mocked out since we're already replacing global.console in jest setup

~~👍 Also updated the jest global config to use noop function instead of jest.fn() as the latter uses system resource while testing though it's not expected to be asserted.~~

The above change would break datastore unit tests which directly uses the original global mock... reverted this change and will do it in a separate PR.

packages/core/__tests__/BackgroundProcessManager.test.ts

ashika112 · 2024-03-19T19:52:30Z

@HuiSF Yeah, actually im seeing some of Chris's comment and i think it makes sense to keep hold of standard but actually to rewrite the tests better within the std.

jimblanc

LGTM, one nit about no-op functions

ashika112 · 2024-03-27T20:34:24Z

packages/core/__tests__/utils/queuedStorage/queuedStorage.test.ts

-				set onerror(_) {},
+				get onsuccess() {
+					return () => {
+						// no op


i still feel this is no-op is excessive and too verbose on tests

Please enter the commit message for your changes. Lines starting

This reverts commit 913b11e.

… core (#13108) * chore(repo): setup the base for lint __tests__ codebase * chore(adapter-nextjs): run yarn lint:fix * chore(adapter-nextjs): manual fix linter reported erros * chore(aws-amplify): run yarn lint:fix Please enter the commit message for your changes. Lines starting * chore(aws-amplify): add camelcase var name exception: phone_number * chore(core): run yarn lint:fix * chore(core): manual fix linter reported errors under __tests__ * apply suggestions * turn off node callback function convention check * do not mock console functions globaly but noopify them * apply suggestions * Revert "do not mock console functions globaly but noopify them" This reverts commit 913b11e. * refactor a no-op function * remove // eslint-disable-next-line no-new

HuiSF requested review from a team as code owners March 11, 2024 22:13

HuiSF force-pushed the hui/chore/lint-test-suite-1 branch from 7cb4d22 to 857f60a Compare March 11, 2024 22:17

svidgen previously approved these changes Mar 12, 2024

View reviewed changes

ashika112 reviewed Mar 15, 2024

View reviewed changes

packages/core/__tests__/BackgroundProcessManager.test.ts Outdated Show resolved Hide resolved

ashika112 reviewed Mar 15, 2024

View reviewed changes

packages/core/__tests__/utils/queuedStorage/queuedStorage.native.test.ts Outdated Show resolved Hide resolved

ashika112 reviewed Mar 15, 2024

View reviewed changes

packages/core/__tests__/clients/composeApiHandler.test.ts Outdated Show resolved Hide resolved

HuiSF dismissed svidgen’s stale review via 45c0c34 March 18, 2024 22:42

HuiSF force-pushed the hui/chore/lint-test-suite-1 branch from 857f60a to 45c0c34 Compare March 18, 2024 22:42

HuiSF requested review from cshfang, svidgen and ashika112 March 18, 2024 22:42

cshfang reviewed Mar 19, 2024

View reviewed changes

HuiSF force-pushed the hui/chore/lint-test-suite-1 branch from 45c0c34 to 23d98bc Compare March 19, 2024 21:45

HuiSF requested a review from cshfang March 19, 2024 21:49

HuiSF force-pushed the hui/chore/lint-test-suite-1 branch 2 times, most recently from 4fc0331 to 4c7e4af Compare March 20, 2024 23:40

jimblanc previously approved these changes Mar 21, 2024

View reviewed changes

HuiSF force-pushed the hui/chore/lint-test-suite-1 branch from 4c7e4af to ea6b47d Compare March 26, 2024 22:53

cshfang previously approved these changes Mar 26, 2024

View reviewed changes

HuiSF dismissed stale reviews from cshfang and jimblanc via a009e7f March 26, 2024 23:48

HuiSF force-pushed the hui/chore/lint-test-suite-1 branch from ea6b47d to a009e7f Compare March 26, 2024 23:48

cshfang approved these changes Mar 26, 2024

View reviewed changes

ashika112 approved these changes Mar 27, 2024

View reviewed changes

ashika112 reviewed Mar 27, 2024

View reviewed changes

jimblanc approved these changes Mar 28, 2024

View reviewed changes

HuiSF added 14 commits March 28, 2024 10:26

chore(repo): setup the base for lint __tests__ codebase

dff5b16

chore(adapter-nextjs): run yarn lint:fix

187df13

chore(adapter-nextjs): manual fix linter reported erros

766073f

chore(aws-amplify): run yarn lint:fix

d66a58e

Please enter the commit message for your changes. Lines starting

chore(aws-amplify): add camelcase var name exception: phone_number

c70bae3

chore(core): run yarn lint:fix

f899b97

chore(core): manual fix linter reported errors under __tests__

cb03295

apply suggestions

aaf47f9

turn off node callback function convention check

9bc046e

do not mock console functions globaly but noopify them

0d9a8a0

apply suggestions

1e7d3ca

Revert "do not mock console functions globaly but noopify them"

7b41395

This reverts commit 913b11e.

refactor a no-op function

cf8bc04

remove // eslint-disable-next-line no-new

1a830db

HuiSF force-pushed the hui/chore/lint-test-suite-1 branch from a009e7f to 1a830db Compare March 28, 2024 17:26

HuiSF merged commit c4f1432 into main Mar 28, 2024
30 checks passed

HuiSF deleted the hui/chore/lint-test-suite-1 branch March 28, 2024 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: enable eslint for tests for adapter-nextjs aws-amplify and core #13108

chore: enable eslint for tests for adapter-nextjs aws-amplify and core #13108

HuiSF commented Mar 11, 2024

svidgen left a comment

svidgen Mar 12, 2024

ashika112 Mar 15, 2024

HuiSF Mar 15, 2024 •

edited

Loading

cshfang Mar 15, 2024

ashika112 Mar 19, 2024

svidgen Mar 27, 2024

ashika112 Mar 15, 2024

ashika112 commented Mar 15, 2024

HuiSF commented Mar 15, 2024

cshfang Mar 19, 2024

HuiSF Mar 19, 2024

cshfang Mar 19, 2024

HuiSF Mar 19, 2024 •

edited

Loading

HuiSF Mar 20, 2024

ashika112 commented Mar 19, 2024

jimblanc left a comment

ashika112 Mar 27, 2024

		manager.add(async () => new Promise(unsleep => setTimeout(unsleep, 10)));
		manager.add(async () => new Promise(resolve => setTimeout(resolve, 10)));

chore: enable eslint for __tests__ for adapter-nextjs aws-amplify and core #13108

chore: enable eslint for __tests__ for adapter-nextjs aws-amplify and core #13108

Conversation

HuiSF commented Mar 11, 2024

Description of changes

Issue #, if available

Description of how you validated changes

Checklist

svidgen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HuiSF Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashika112 commented Mar 15, 2024

HuiSF commented Mar 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HuiSF Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashika112 commented Mar 19, 2024

jimblanc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chore: enable eslint for tests for adapter-nextjs aws-amplify and core #13108

chore: enable eslint for tests for adapter-nextjs aws-amplify and core #13108

HuiSF Mar 15, 2024 •

edited

Loading

HuiSF Mar 19, 2024 •

edited

Loading