#4734 Use separate mock api for each test #4928

sosnovsky · 2023-02-01T12:34:00Z

This PR implements separate mock api for each test:

nodejs finds available port for new mock api
we create copy of extension build which uses new port
then this port is passed to tests using AvaContext

close #4734

Tests (delete all except exactly one):

Tests added or updated

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

addresses the issue it closes (if any)
code is readable and understandable
is accompanied with tests, or tests are not needed
is free of vulnerabilities
is documented clearly and usefully, or doesn't need documentation

tomholub

👍 that's a lot of tests!

Looking good.

extension/chrome/elements/pgp_block_modules/pgp-block-render-module.ts

test/source/browser/browser-pool.ts

test/source/test.ts

test/source/tests/tooling/index.ts

ioanmo226

Looks great 👍

test/source/mock/fes/shared-tenant-fes-endpoints.ts

ioanmo226 · 2023-02-01T15:04:46Z

By the way, I can see that test sometimes fails with below error. Need to be investigated and fixed.

message: failed to log into google in newGoogleAcctAuthPromptThenAlertOrForward14:38
url: chrome-extension://clinllignhmgdgkljgkdckcebolglnbf/chrome/settings/index.htm?account_email=test-upd14:38
trace: Error: failed to log into google in newGoogleAcctAuthPromptThenAlertOrForward14:38
    at Catch.nameAndDetailsAsException (chrome-extension://clinllignhmgdgkljgkdckcebolglnbf/js/common/platform/catch.js:358:15)14:38
    at Catch.report (chrome-extension://clinllignhmgdgkljgkdckcebolglnbf/js/common/platform/catch.js:102:34)14:38
    at Settings.newGoogleAcctAuthPromptThenAlertOrForward (chrome-extension://clinllignhmgdgkljgkdckcebolglnbf/js/common/settings.js:388:27)14:38
    at async SettingsView.<anonymous> (chrome-extension://clinllignhmgdgkljgkdckcebolglnbf/chrome/settings/index.js:204:28)14:38
details:14:38
[typeof:object:[object Object]] {"result":"Error","error":"Grant successful but error accessing fc account: Error: Bad Request: 400 when GET-ing https://localhost:40743/shared-tenant-fes/api/v1/client-configuration?domain=settings.flowcrypt.test (no body):  -> Test error","acctEmail":"test-update@settings.flowcrypt.test"}14:38
### Catch.reportErr calling stack ###14:38
#     at Catch.formatExceptionForReport (chrome-extension://clinllignhmgdgkljgkdckcebolglnbf/js/common/platform/catch.js:272:90)14:38
#     at Catch.onErrorInternalHandler (chrome-extension://clinllignhmgdgkljgkdckcebolglnbf/js/common/platform/catch.js:80:29)14:38
#     at Catch.reportErr (chrome-extension://clinllignhmgdgkljgkdckcebolglnbf/js/common/platform/catch.js:96:18)14:38
#     at Catch.report (chrome-extension://clinllignhmgdgkljgkdckcebolglnbf/js/common/platform/catch.js:102:18)14:38
#     at Settings.newGoogleAcctAuthPromptThenAlertOrForward (chrome-extension://clinllignhmgdgkljgkdckcebolglnbf/js/common/settings.js:388:27)14:38
#     at async SettingsView.<anonymous> (chrome-extension://clinllignhmgdgkljgkdckcebolglnbf/chrome/settings/index.js:204:28)14:38
# 14:38
# url: chrome-extension://clinllignhmgdgkljgkdckcebolglnbf/chrome/settings/index.htm?account_email=test-update@settings.flowcrypt.test

sosnovsky · 2023-02-01T19:58:40Z

By the way, I can see that test sometimes fails with below error. Need to be investigated and fixed.

@ioanmo226 I think it happened once, test passes well now, but let's monitor if it'll fail in the future.
Sometimes there was an issue with test execution time limit, I increased it a bit, seems like it's enough for now.

rrrooommmaaa · 2023-02-12T10:01:49Z

package.json

+    "test_ci_chrome_consumer_live_gmail": "npx ava --timeout=20m --verbose --concurrency=10 build/test/test/source/test.js -- CONSUMER-LIVE-GMAIL STANDARD-GROUP",
+    "test_ci_chrome_consumer": "npx ava --timeout=20m --verbose --concurrency=10 build/test/test/source/test.js -- CONSUMER-MOCK STANDARD-GROUP",
+    "test_ci_chrome_enterprise": "npx ava --timeout=20m --verbose --concurrency=10 build/test/test/source/test.js -- ENTERPRISE-MOCK STANDARD-GROUP",
+    "test_ci_chrome_consumer_flaky": "npx ava --timeout=20m --verbose --concurrency=10 build/test/test/source/test.js -- CONSUMER-MOCK FLAKY-GROUP",


What is the purpose of the "flaky" group? These are resource-demanding tests or tests with unstable performance. This group is meant for tests to be run consequently, one at a time

Some tests are inexplicably resource-demanding. When run in parallel with other tests, what can happen is that either that test will time out, or other tests will fail while the resource-intensive test hogs up resources.

Other reason that a test may end up in "flaky" section is if it needs to be executed serially for some other reason.

Generally speaking, these tests tend to cause reliability trouble when executed in parallel, for one reason or another.

Of course, they could be debugged and fixed one by one and moved, possibly, if we had the time or willingness.

Does this mean we should set --concurrency=1 instead of --concurrency=10?

Yes, we can change it back to --concurrency=1 as browser pool size is automatically set to 1 for FLAKY-GROUP - https://github.com/FlowCrypt/flowcrypt-browser/blob/master/test/source/util/index.ts#L31

Shouldn't browser pool and concurrency always be exactly equal? Or is there a reason to have these two numbers diverge? I'd imagine, we could remove special code to choose that FLAKY-GROUP has 1 in the pool, and instead follow whatever the provided --concurrency ?

Probably it can be simplified, as currently --pool-size parameter is used for determining oneIfNotPooled, which then also used for setting ATTEMPTS constant - https://github.com/FlowCrypt/flowcrypt-browser/blob/master/test/source/test.ts#L40

rrrooommmaaa · 2023-02-12T10:03:22Z

scripts/config-mock-build.sh

+BUILD_PATH=./build/test/mock-builds/port-$2-$(openssl rand -hex 12)
+mkdir -p $BUILD_PATH
+cp -r ./$1/* $BUILD_PATH
+grep ":8001" $BUILD_PATH -lr | xargs sed -i.bak -e "s/\:8001/\:$2/g"


Can it be solved with a [BUILD_REPLACEABLE_....] placeholder?

I suppose it could.. we would still need to do two rounds of replacing? The step that normally replaces [BUILD_REPLACEABLE_....] and currently puts the :8001 would still need to insert some more maybe [TEST_REPLACEABLE_MOCK_PORT] and then that would be replaced here.

It's true that as things are now, there is no meaning to the :8001 in particular. So that should not be put in, to not be confusing. So [TEST_REPLACEABLE_MOCK_PORT] or similar would be a better choice here.

sosnovsky added 30 commits January 13, 2023 12:05

remove unnecessary eslint-disable

88e04cd

Merge branch 'master' into feature/issue-4734-one-mock-per-test

957c9c0

Merge branch 'master' into feature/issue-4734-one-mock-per-test

808c3b5

choose random port for tests

6341593

improve test port setting

bc37919

add default mock api

e930393

separate port for each test

f24f68c

Merge branch 'master' into feature/issue-4734-one-mock-per-test

0fa7882

get extension id once

a6e8f6f

Merge branch 'master' into feature/issue-4734-one-mock-per-test

9de3ba3

Merge branch 'master' into feature/issue-4734-one-mock-per-test

1b10bed

Merge branch 'master' into feature/issue-4734-one-mock-per-test

be0f6e1

make tests independent

cffd6f9

Merge branch 'master' into feature/issue-4734-one-mock-per-test

61b7385

fix extensionDir path

e856a3c

update setMaxListeners

1d3ba2e

add debug info

23b6016

more logs

44f5901

update WKD_API_HOST

431ea85

update consts

5e76fab

debug

eb82ec3

test ContactStore

6c5a2db

run all unit tests

aefb067

run only unit tests

e01e558

fix unit mock port

cf52c48

run all wkd tests

f97c74b

separate gmail tests

f00a3b7

disable gmail

92ba040

disable gmail

bded044

remove acct param

e266f15

sosnovsky added 10 commits January 31, 2023 11:50

fix mock email parsing

5569bd2

code cleanup

9adca86

fix mock errors handling

87dab76

code cleanup

963aa56

code cleanup

6cb9221

increase concurrency

f796a3f

Merge branch 'master' into feature/issue-4734-one-mock-per-test

6e5370d

close mock api on test end

94503e3

fix mock api close

33c6860

code cleanup

efe8f74

sosnovsky requested review from tomholub and ioanmo226 February 1, 2023 12:38

sosnovsky marked this pull request as ready for review February 1, 2023 12:38

sosnovsky requested a review from rrrooommmaaa as a code owner February 1, 2023 12:38

tomholub previously approved these changes Feb 1, 2023

View reviewed changes

remove closeMockApi from AvaContext

9851558

sosnovsky dismissed tomholub’s stale review via 9851558 February 1, 2023 13:18

move closeMockApi to try/finally

27033c6

sosnovsky mentioned this pull request Feb 1, 2023

Improve tests performance by testing iframes directly #4929

Closed

tomholub approved these changes Feb 1, 2023

View reviewed changes

tomholub merged commit c8485e4 into master Feb 1, 2023

tomholub deleted the feature/issue-4734-one-mock-per-test branch February 1, 2023 14:54

ioanmo226 approved these changes Feb 1, 2023

View reviewed changes

test/source/mock/fes/shared-tenant-fes-endpoints.ts Show resolved Hide resolved

rrrooommmaaa reviewed Feb 12, 2023

View reviewed changes

tomholub mentioned this pull request Feb 13, 2023

stop adding then replacing port 8001 in mock builds #4953

Open

sosnovsky mentioned this pull request Feb 13, 2023

Use --concurrency param for browser pool #4955

Open

rrrooommmaaa mentioned this pull request Feb 13, 2023

issue #4494 Allow endpoint handlers configurations from inside a test #4952

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#4734 Use separate mock api for each test #4928

#4734 Use separate mock api for each test #4928

sosnovsky commented Feb 1, 2023 •

edited by ioanmo226

tomholub left a comment

ioanmo226 left a comment

ioanmo226 commented Feb 1, 2023

sosnovsky commented Feb 1, 2023

rrrooommmaaa Feb 12, 2023 •

edited

tomholub Feb 13, 2023

rrrooommmaaa Feb 13, 2023

sosnovsky Feb 13, 2023

tomholub Feb 13, 2023

sosnovsky Feb 13, 2023

sosnovsky Feb 13, 2023

rrrooommmaaa Feb 12, 2023

tomholub Feb 13, 2023 •

edited

#4734 Use separate mock api for each test #4928

#4734 Use separate mock api for each test #4928

Conversation

sosnovsky commented Feb 1, 2023 • edited by ioanmo226

To be filled by reviewers

tomholub left a comment

Choose a reason for hiding this comment

ioanmo226 left a comment

Choose a reason for hiding this comment

ioanmo226 commented Feb 1, 2023

sosnovsky commented Feb 1, 2023

rrrooommmaaa Feb 12, 2023 • edited

Choose a reason for hiding this comment

tomholub Feb 13, 2023

Choose a reason for hiding this comment

rrrooommmaaa Feb 13, 2023

Choose a reason for hiding this comment

sosnovsky Feb 13, 2023

Choose a reason for hiding this comment

tomholub Feb 13, 2023

Choose a reason for hiding this comment

sosnovsky Feb 13, 2023

Choose a reason for hiding this comment

sosnovsky Feb 13, 2023

Choose a reason for hiding this comment

rrrooommmaaa Feb 12, 2023

Choose a reason for hiding this comment

tomholub Feb 13, 2023 • edited

Choose a reason for hiding this comment

sosnovsky commented Feb 1, 2023 •

edited by ioanmo226

rrrooommmaaa Feb 12, 2023 •

edited

tomholub Feb 13, 2023 •

edited