chore(saved-aggregations-queries): Use proxyquire to mock dependencies that cause misc issues for running tests in the package

[gribnoysup]

This configuration will resolve anything with the matching path in the __mocks__ folder first before trying to do a real import. I also added a simple test case for the connected list to demonstrate how this can be used to provide different values during the test run. The top level __mocks__ folder name is a pattern used by jest to implement a similar behavior. The intended use is fully mocking whole dependencies of a tested package, for simple spying or mocking of module methods, we probably should still stick to sinon as this seems to be the best tool for the job. decided not to go with this

Alternatively if we don't want to go with this approach, we can use something like proxyquire, but for the use-case where the whole module needs to be mocked and never imported it does require a very verbose configuration that would look sorta like this:

import { expect } from 'chai';
import proxyquire from 'proxyquire';

const CompassPlugin = proxyquire.load('./index.ts', {
  '@mongodb-js/compass-query-history': {
    FavoriteQueryStorage: class FavoriteQueryStorageMock {},
    '@global': true,
    '@noCallThru': true,
  },
  '@mongodb-js/compass-aggregations': {
    readPipelinesFromStorage() {},
    '@global': true,
    '@noCallThru': true,
  },
});

describe('Compass Plugin', function () {
  it('exports activate, deactivate, and metadata', function () {
    expect(CompassPlugin).to.have.property('activate');
    expect(CompassPlugin).to.have.property('deactivate');
    expect(CompassPlugin).to.have.property('metadata');
  });
});

We would need to use proxyquire explicitly for every import that has dependencies that we want to mock in the dependency tree. It also doesn't play well with typescript and only works with commonjs modules (which can be circumvented by forcing ts-node to compile everything we run through it to commonjs)

[mcasimir]

This one is quite neat and can mock interfaces on demand too. I see how for compass-saved-aggregations-queries this can be convenient.

If I have to pick one in general for the entire Compass I would "like" the proxyquire approach a bit better, seems more flexible and intentional as we could mock everything in a test suite and nothing (or only something) in another. It also appeals a bit to me that we already use it somewhere else so we would avoid introducing another pattern.

To be really really honest I don't love either, but for compass-saved-aggregations-queries we could live with any of those.

As for the bloat and repetition I don't see a significant difference, we could work that out in the usual way we factor code:

// tests/mocks.ts

export const mockCompassPlugin = (FavoriteQueryStorageMock, readPipelinesFromStorageMock) => proxyquire.load('./index.ts', {
  '@mongodb-js/compass-query-history': {
    FavoriteQueryStorage: FavoriteQueryStorageMock,
    '@global': true,
    '@noCallThru': true,
  },
  '@mongodb-js/compass-aggregations': {
    readPipelinesFromStorage: readPipelinesFromStorageMock,
    '@global': true,
    '@noCallThru': true,
  },
});

// my-test.spec.ts

import {mockCompassPlugin} from './mocks';
const CompassPlugin = mockCompassPlugin(mockThis, mockThat);

[gribnoysup]

If I have to pick one in general for the entire Compass I would "like" the proxyquire approach a bit better

I'm happy to change it to whatever will allow us to finally start writing tests for this plugin 👍

To be really really honest I don't love either, but for compass-saved-aggregations-queries we could live with any of those.

How would the ideal case look for you? I'm feeling like I'm loosing track of what we are trying to achieve here. From my perspective: I want to write tests for the connected component without it needing to actually interact with these "models" that use file system and electron apis, and require mocking even for transitive dependencies, I also want to easily change the values returned by these models so I can test different scenarios, both of those are achieved with these mocking approaches (one with a bit clunkier interface, but I can live with that)

[mcasimir]

I wish I had a stronger opinion for this choice, if we need to pick one for now and is ok with you let's go with proxyquire, with the other approach I'm only slightly afraid that we would need (or wish) to "skip mocking" at some levels (not in this specific case) and then we would need to have both the __mocks__ approach and the proxyquire one.

My favorite one would definitely go through providers and avoid proxyquire and messing with requires and imports altogether for this kind of dependencies. For example, the context-like approach you mentioned would probably be something I'd like. But we can just unblock this one for now.

[gribnoysup]

Gotcha, makes sense! Thanks for sharing your thoughts on this

[gribnoysup]

This still doesn't fix all the issues with different instances being in scope of the mocked import and outside of it, for example @emotion/react still resolves multiple times and prints a warning about that, but this doesn't seem to cause any real issues in this particular test case so seems alright to ignore for now. To fix it completely we would need to pass all the dependencies of the plugin we are mocking manually

[gribnoysup]

Changed to use proxyquire, do you want to give it another look @mcasimir ?

[gribnoysup]

Unfortunately the time to run the test is quite slow again because the way proxyquire mocks things still requires ts-node to process the file, it just doesn't run it, (~5000ms vs ~50ms compared to complete mock of the package in the previous implementation, but still faster than before as the code is not evaluated) so we might need to bump the timeout for this test suite if it starts flaking on imports

Yep, it did fail, bumped the timeouts, let's see if it passes now

[mcasimir]

SGTM, let's hope that we can move the *storage to more appropriate place and use providers, if what's in useTheme useToast works out, that's so much nicer.