Browse Source

feat(tests): lazy load stubs (#40428)

I identified a performance bottleneck by profiling our jest tests -
loadFixtures function eagerly loads all files (~100) and requires all of
their modules even if they are not needed for a specific test.

**Profile before:**
<img width="1065" alt="CleanShot 2022-10-22 at 15 12 47@2x"
src="https://user-images.githubusercontent.com/9317857/197358564-c135c701-f4bb-4c7c-afad-31b2d10a6ec2.png">

The solution is to wrap the fixtures object and lazily load fixtures as
they are accessed (using proxies). Since they dont map exactly 1:1 to
files on disk I hardcoded a few exceptions.

**Profile after**
<img width="1060" alt="CleanShot 2022-10-22 at 15 17 43@2x"
src="https://user-images.githubusercontent.com/9317857/197358698-da16796c-2c77-40d8-b598-ee2ec6c362b0.png">

Table from profiling tab confirms the improvement
<img width="943" alt="CleanShot 2022-10-22 at 15 17 03@2x"
src="https://user-images.githubusercontent.com/9317857/197358754-18786f7c-de89-4853-bec6-ae99b0c8cd6c.png">

Since setup.ts is executed
[once](https://jestjs.io/docs/configuration#setupfiles-array) for each
file I would expect this to have a pretty large impact on the duration
of the entire test suite. We have ~400 test files and if we take a
conservative measurement of 500ms per test that should yield about ~3m
worth of improvement on our entire test suite.

Since that does not seem to be the case, I am wondering if jest was not
doing some caching on loadFixtures already and that is why we are not
seeing tests run faster?

Something to eliminate is to check if the preloading times have not
simply been moved over into the tests themselves. Even if that is the
case, I would still expect an improvement on the test duration (unless
we somehow still always end up loading the entire set of fixtures
somehow)
Jonas 2 years ago
parent
commit
989d09d3d3

+ 1 - 1
fixtures/js-stubs/integrationListDirectory.js

@@ -55,7 +55,7 @@ export function BitbucketIntegrationConfig() {
   };
 }
 
-export function GithubIntegrationConfig() {
+export function GitHubIntegrationConfig() {
   return {
     accountType: null,
     configData: {},

+ 1 - 1
fixtures/js-stubs/types.tsx

@@ -47,9 +47,9 @@ type TestStubFixtures = {
   EventsStats: OverridableStub;
   ExceptionWithMeta: OverridableStubList;
   GitHubIntegration: OverridableStub;
+  GitHubIntegrationConfig: SimpleStub;
   GitHubIntegrationProvider: OverridableStub;
   GitHubRepositoryProvider: OverridableStub;
-  GithubIntegrationConfig: SimpleStub;
   GlobalSelection: OverridableStub;
   Group: OverridableStub;
   GroupStats: OverridableStub;

+ 0 - 1
jest.config.ts

@@ -1,6 +1,5 @@
 /* eslint-env node */
 /* eslint import/no-nodejs-modules:0 */
-
 import path from 'path';
 import process from 'process';
 

+ 1 - 1
static/app/views/settings/project/projectOwnership/index.spec.jsx

@@ -29,7 +29,7 @@ describe('Project Ownership', () => {
       url: `/organizations/${org.slug}/integrations/`,
       query: {features: 'codeowners'},
       method: 'GET',
-      body: [TestStubs.GithubIntegrationConfig()],
+      body: [TestStubs.GitHubIntegrationConfig()],
     });
     MockApiClient.addMockResponse({
       url: `/projects/${org.slug}/${project.slug}/codeowners/`,

+ 97 - 0
tests/js/sentry-test/loadFixtures.ts

@@ -69,3 +69,100 @@ export function loadFixtures(dir: string, opts: Options = {}): TestStubFixtures
 
   return fixtures;
 }
+
+const extensions = ['.js', '.ts', '.tsx', '.json'];
+
+// This is a mapping of special cases where fixture name does not map 1:1 to file name.
+// Some fixture files also contain more than one fixture so additional mappings are needed.
+// If you have added new fixtures and you are seeing an error being throw, please add the fixture
+const SPECIAL_MAPPING = {
+  AllAuthenticators: 'authenticators.js',
+  OrgRoleList: 'roleList.js',
+  MetricsField: 'metrics.js',
+  EventsStats: 'events.js',
+  DetailedEvents: 'events.js',
+  Events: 'events.js',
+  OutcomesWithReason: 'outcomes.js',
+  SentryAppComponentAsync: 'sentryAppComponent.js',
+  EventStacktraceMessage: 'eventStacktraceException.js',
+  MetricsTotalCountByReleaseIn24h: 'metrics.js',
+  MetricsSessionUserCountByStatusByRelease: 'metrics.js',
+  MOCK_RESP_VERBOSE: 'ruleConditions.js',
+  SessionStatusCountByProjectInPeriod: 'sessions.js',
+  SessionUserCountByStatusByRelease: 'sessions.js',
+  SessionUserCountByStatus: 'sessions.js',
+  SessionStatusCountByReleaseInPeriod: 'sessions.js',
+  SessionsField: 'sessions.js',
+  ProviderList: 'integrationListDirectory.js',
+  BitbucketIntegrationConfig: 'integrationListDirectory.js',
+  GitHubIntegration: 'githubIntegration.js',
+  GitHubRepositoryProvider: 'githubRepositoryProvider.js',
+  GitHubIntegrationProvider: 'githubIntegrationProvider.js',
+  GitHubIntegrationConfig: 'integrationListDirectory.js',
+  OrgOwnedApps: 'integrationListDirectory.js',
+  PublishedApps: 'integrationListDirectory.js',
+  SentryAppInstalls: 'integrationListDirectory.js',
+  PluginListConfig: 'integrationListDirectory.js',
+  DiscoverSavedQuery: 'discover.js',
+  VercelProvider: 'vercelIntegration.js',
+  TagValues: 'tagvalues.js',
+};
+
+function tryRequire(dir: string, name: string): any {
+  if (SPECIAL_MAPPING[name]) {
+    return require(path.resolve(dir, SPECIAL_MAPPING[name]));
+  }
+  for (const ext of extensions) {
+    try {
+      return require(path.resolve(dir, lowercaseFirst(name) + ext));
+    } catch {
+      // ignore
+    }
+  }
+  throw new Error('Failed to resolve file');
+}
+
+function lowercaseFirst(value: string): string {
+  return value.charAt(0).toLowerCase() + value.slice(1);
+}
+export function makeLazyFixtures<UserProvidedFixtures extends Record<any, any>>(
+  fixturesDirectoryPath: string,
+  userProvidedFixtures: UserProvidedFixtures
+): TestStubFixtures & UserProvidedFixtures {
+  const lazyFixtures = new Proxy(
+    {},
+    {
+      get(target, prop: string) {
+        if (target[prop]) {
+          return target[prop];
+        }
+        if (userProvidedFixtures[prop]) {
+          return userProvidedFixtures[prop];
+        }
+
+        try {
+          const maybeModule = tryRequire(fixturesDirectoryPath, prop);
+          for (const exportKey in maybeModule) {
+            target[exportKey] = maybeModule[exportKey];
+          }
+        } catch {
+          // ignore
+        }
+
+        if (target[prop] === undefined) {
+          return () => {
+            throw new Error(
+              `Failed to resolve ${prop} fixture. \n
+              - Your fixture does not map directly to file on disk or fixture file could be exporting > 1 fixture. \n
+              - To resolve this, add a mapping to SPECIAL_MAPPING in loadFixtures.ts or ensure fixture export name maps to the file on disk. \n
+              - If you are seeing this only in CI and you have followed the step above, check the exact casing of the file as it is case sensitive.`
+            );
+          };
+        }
+        return target[prop];
+      },
+    }
+  );
+
+  return lazyFixtures as TestStubFixtures & UserProvidedFixtures;
+}

+ 10 - 15
tests/js/setup.ts

@@ -1,5 +1,6 @@
 /* eslint-env node */
 /* eslint import/no-nodejs-modules:0 */
+import path from 'path';
 import {TextDecoder, TextEncoder} from 'util';
 
 import {InjectedRouter} from 'react-router';
@@ -15,9 +16,7 @@ import * as qs from 'query-string';
 import type {Client} from 'sentry/__mocks__/api';
 import ConfigStore from 'sentry/stores/configStore';
 
-import TestStubFixtures from '../../fixtures/js-stubs/types';
-
-import {loadFixtures} from './sentry-test/loadFixtures';
+import {makeLazyFixtures} from './sentry-test/loadFixtures';
 
 // needed by cbor-web for webauthn
 window.TextEncoder = TextEncoder;
@@ -56,16 +55,9 @@ configureEnzyme({adapter: new Adapter()});
 const constantDate = new Date(1508208080000);
 MockDate.set(constantDate);
 
-/**
- * Load all files in `tests/js/fixtures/*` as a module.
- * These will then be added to the `TestStubs` global below
- */
-const fixtures = loadFixtures('js-stubs', {flatten: true});
-
 /**
  * Global testing configuration
  */
-ConfigStore.loadInitialData(fixtures.Config());
 
 /**
  * Mocks
@@ -204,8 +196,8 @@ const routerFixtures = {
     context: {
       location: TestStubs.location(),
       router: TestStubs.router(),
-      organization: fixtures.Organization(),
-      project: fixtures.Project(),
+      organization: TestStubs.Organization(),
+      project: TestStubs.Project(),
       ...context,
     },
     childContextTypes: {
@@ -218,7 +210,10 @@ const routerFixtures = {
   }),
 };
 
-type TestStubTypes = TestStubFixtures & typeof routerFixtures;
+const jsFixturesDirectory = path.resolve(__dirname, '../../fixtures/js-stubs/');
+const fixtures = makeLazyFixtures(jsFixturesDirectory, routerFixtures);
+
+ConfigStore.loadInitialData(fixtures.Config());
 
 /**
  * Test Globals
@@ -229,7 +224,7 @@ declare global {
    * directory. Use these for setting up test data.
    */
   // eslint-disable-next-line no-var
-  var TestStubs: TestStubTypes;
+  var TestStubs: typeof fixtures;
   /**
    * Generates a promise that resolves on the next macro-task
    */
@@ -242,7 +237,7 @@ declare global {
   var MockApiClient: typeof Client;
 }
 
-window.TestStubs = {...fixtures, ...routerFixtures};
+window.TestStubs = fixtures;
 
 // This is so we can use async/await in tests instead of wrapping with `setTimeout`.
 window.tick = () => new Promise(resolve => setTimeout(resolve));