mock tests should have inputs configured in the test + one mock API per test

[tomholub]

Currently, the test codebase has a single API mock for all tests, and some parts of the mock setup are meant for some tests while other parts for others, in non-obvious ways. The better example is when there is a comment like this:

      const sentMsgs = (await GoogleData.withInitializedData(acct)).searchMessagesBySubject(subject);
      expect(sentMsgs.length).to.equal(2);
      // this test is using PwdEncryptedMessageWithFesIdTokenTestStrategy to check sent result based on subject "PWD encrypted message with FES - ID TOKEN"
      // also see '/api/v1/message' in fes-endpoints.ts mock

The worse example is silently implied setup, like certain tests expect client configuration to be set up based on their domain, the example is here:

  ava.default('user2@standardsubdomainfes.test - PWD encrypted message with FES - Reply rendering', testWithBrowser(undefined, async (t, browser) => {
      const acct = 'user2@standardsubdomainfes.test'; // added port to trick extension into calling the mock
      const settingsPage = await BrowserRecipe.openSettingsLoginApprove(t, browser, acct);

The mock API endpoints test for certain conditions etc - none of which is obvious from the test.

  '/api/v1/message': async ({ body }, req) => {
      // ...
      if (body.includes('"from":"user2@standardsubdomainfes.test"')) {
        // ...
      }

There's also some that may not apply to this particular test, and may be causing side effects:

    if (req.headers.host === standardFesUrl && req.url === `/api/v1/client-configuration?domain=standardsubdomainfes.test`) {
      return {
        clientConfiguration: { disallow_attester_search_for_domains: ['got.this@fromstandardfes.test'] },
      };

But some other test may rely on that, etc.. it really is spaghetti.

---

I'd like to follow what we do on iOS - one mock API per test, which is configured directly in the test definition with no side effects on other tests. It would look something like this:

  // testWithBrowser would create browser extension build that communicates to a particular port for mock api
  ava.default('some test', testWithBrowser(undefined, assignedPort, async (t, browser) => { 
    const mockApi = new MockApi();
    mockApi.fesConfig = { ... }; // particular client configuration for this test
    mockApi.ekmConfig = { returnKeys: [ekmKeySamples.e2eValidKey.prv] } // same
    mockApi.attesterConfig = { ... }; // configure Attester with particular public keys for this test 
    mockApi.googleConfig = {
      accounts: {
        'e2e.enterprise.test@flowcrypt.com': { // which account is allowed for Gmail login in this test
          messages: ['CC and BCC test'], // exported messages pre-loaded for this test, by subject
        }
      }
    }
    await mockApi.withMockedApis(assignedPort, async () => { // runs the API at a particular port for this test
      // run browser tests as before
      // ...
      // ... composeScreen.fillAndSendMsg(...);
      // ...
      // now test the sent message - this replaces all of the "test strategy" classes
      expect(mockApi.google.sentMessages.length).to.equal(1);
      expect(mockApi.google.sentMessages[0].parsed.subject).to.equal(...);
      expect(mockApi.google.sentMessages[0].parsed.attachments.length).to.equal(0);
      const decryptedBody = decrypt(mockApi.google.sentMessages[0].parsed.body, ekmKeySamples.e2eValidKey.prv);
      expect(decryptedBody).to.equal(...);
      // ... end so on
    });
  });
});

---

The steps would be as follows:

First PR: for each test, we will have to (probably inside testWithBrowser):

• copy browser extension folder into a new place
• get a fresh unused port
• regex-replace the mock api port in the extension
• load this particular extension for the test

  ava.default('some test', testWithBrowser(undefined, assignedPort, async (t, browser) => { 
    const mockApi = new MockApi();
    await mockApi.withMockedApis(assignedPort, async () => {
      // run browser tests as before

Second wave of PRs: allow local configuration of the mock APIs, but give every test the same configuration:

I would recommend to do this service by service, eg start with only Attester, then FES in another PR, etc.

  ava.default('some test', testWithBrowser(undefined, assignedPort, async (t, browser) => { 
    const mockApi = new MockApi();
    mockApi.fesConfig = LEGACY_GLOBAL_FES_MOCK_CONFIG;
    mockApi.ekmConfig = LEGACY_GLOBAL_EKM_MOCK_CONFIG
    mockApi.attesterConfig = LEGACY_GLOBAL_ATTESTER_MOCK_CONFIG;
    mockApi.googleConfig = LEGACY_GLOBAL_GOOGLE_MOCK_CONFIG;
    await mockApi.withMockedApis(assignedPort, async () => {
      // run browser tests as before

This way, we ensure that all current schenanigans we do for tests can be configured using the new mechanism. But we don't yet do individual configuration per test, because it's a task on its own to figure out which tests need exactly what configuration.

Third wave of PRs: start dis-entangling the dependencies for each test from the LEGACY_GLOBAL configs:

    const mockApi = new MockApi();
    mockApi.fesConfig = { ... }; // particular client configuration for this test
    mockApi.ekmConfig = { ... };
    mockApi.attesterConfig = { ... };
    mockApi.googleConfig = { ... };

[tomholub]

I just got to work with the codebase, and the tests truly are unmaintainable as they are today. I'll try to move this (the first step #4734) up in priority.

[tomholub]

Second wave of PRs: allow local configuration of the mock APIs, but give every test the same configuration:

I would recommend to do this service by service, eg start with only Attester, then FES in another PR, etc.

  ava.default('some test', testWithBrowser(undefined, assignedPort, async (t, browser) => { 
    const mockApi = new MockApi();
    mockApi.fesConfig = LEGACY_GLOBAL_FES_MOCK_CONFIG;
    mockApi.ekmConfig = LEGACY_GLOBAL_EKM_MOCK_CONFIG
    mockApi.attesterConfig = LEGACY_GLOBAL_ATTESTER_MOCK_CONFIG;
    mockApi.googleConfig = LEGACY_GLOBAL_GOOGLE_MOCK_CONFIG;
    await mockApi.withMockedApis(assignedPort, async () => {
      // run browser tests as before

This way, we ensure that all current schenanigans we do for tests can be configured using the new mechanism. But we don't yet do individual configuration per test, because it's a task on its own to figure out which tests need exactly what configuration.

[rrrooommmaaa]

I don't like this global variable for collecting logs.

flowcrypt-browser/test/source/test.ts

Line 54 in c52ae09

const mockApiLogs: string[] = [];

Log lines may get mixed up from various tests running concurrently, right?
I vaguely remember seeing some inconsistent messages in logs some time ago.

It's hard to retrieve a test's own context in test.after.always as test.after.always is actually running with its own separate ExecutionContext, that's what I found out when debugging.
I think, we can try to chain always to each defined test individually.

@sosnovsky ?

[sosnovsky]

It's hard to retrieve a test's own context in test.after.always as test.after.always is actually running with its own separate ExecutionContext, that's what I found out when debugging.

Yeah, it should be useful for tests debugging to separate mock api logs across different tests.

I think, we can try to chain always to each defined test individually.

You mean using test.afterEach.always or some other solution?

[rrrooommmaaa]

You mean using test.afterEach.always or some other solution?

Yes, this would work (I was mistaken in that test.after.always runs after each test).
With that we can extract context of the failed test, with a bit of refactoring.
Our custom AvaContext class shouldn't extend ExecutionContext like this

flowcrypt-browser/test/source/tests/tooling/index.ts

Lines 9 to 17 in 442ed6e

	export type AvaContext = ExecutionContext<unknown> & {
	retry?: true;
	attemptNumber?: number;
	totalAttempts?: number;
	attemptText?: string;
	extensionDir?: string;
	urls?: TestUrls;
	mockApi?: Api<{ query: { [k: string]: string }; body?: unknown }, unknown>;
	};

instead we should use the ExecutionContext<AvaContext>, so that we'll be able to extract test's context via t.context in afterEach

[sosnovsky]

Sounds good 👍

[tomholub]

I'm closing this master issue, if anything is still left to do, let's file individual issues. Well done.