[chore] set up pre-commit test hook and fixed all broken tests (#23)

Signed-off-by: Giuseppe Macri <[email protected]>

.github/actions/setup/action.yml

@@ -21,7 +21,7 @@ jobs:
         uses: actions/setup-node@v3
         with:
           node-version: ${{ matrix.node-version }}
-          cache: 'yarn'
+          cache: 'npm'
       - name: Install dependencies
         run: yarn install
       - name: Format Check

.github/pull_request_template.md

@@ -9,6 +9,7 @@ If this PR is a work in progress, please prefix the title with [WIP].
 - [] Breaking change (fix or feature that would cause existing functionality to not work as expected)
 - [] Refactor (refactoring production code or docs)
 - [] Test (adding missing tests or fixing existing tests)
+- [] Chore (updating yarn tasks etc; no production code change)
 - [] Other (please describe):
 
 ## Checklist

package.json

@@ -39,7 +39,7 @@
     "eslint-plugin-promise": "^6.0.0",
     "husky": "^8.0.3",
     "jsdom": "^22.1.0",
-    "lint-staged": "^13.2.3",
+    "lint-staged": "^15.2.0",
     "prettier": "^3.0.0",
     "typescript": "*",
     "vite": "^5.0.6",
@@ -48,7 +48,8 @@
   },
   "husky": {
     "hooks": {
-      "pre-commit": "lint-staged"
+      "pre-commit": "lint-staged",
+      "pre-push": "yarn test"
     }
   },
   "lint-staged": {

src/storage/config.ts

@@ -6,12 +6,14 @@ export const DEFAULT_CONFIG = {
   projectName: '',
   isDebug: false,
   onError: () => undefined,
-  eventPath: '/amp',
+  // TODO: all api endpoints should be moved into the network module
+  eventPath: '/events',
   metricPath: '/metrics',
+  apiEndpoint: 'https://cca-lite.coinbase.com', // works for production only
+
   disabled: false,
   isAlwaysAuthed: false,
   version: null,
-  apiEndpoint: 'https://cca-lite.coinbase.com', // works for production only
   ricTimeoutScheduleEvent: 1000,
   // TODO: find better solution to handle reset
   reset: () => {},

src/storage/networkLayer.test.ts

@@ -1,9 +1,9 @@
-import { sendEvents, sendMetrics, createNetworkLayer } from './networkLayer';
+import {createNetworkLayer, sendEvents, sendMetrics} from './networkLayer';
 import { Event } from '../types/event.ts';
 import { Metric, MetricType } from '../types/metric';
 import * as apiFetch from '../utils/apiFetch';
 import * as getChecksum from '../utils/dataIntegrity';
-import { describe, it, expect, vi, SpyInstance, beforeEach } from 'vitest';
+import { describe, test, expect, vi, MockInstance, beforeEach } from 'vitest';
 import { getConfig, getIdentity } from './storage';
 import { init as setConfig } from './config';
 
@@ -14,9 +14,9 @@ describe('networkLayer', () => {
       { action: 'hover', component: 'unknown', name: 'defaultEvent2' },
     ];
 
-    let apiFetchSpy: SpyInstance;
-    let configOnErrorSpy: SpyInstance;
-    let checksumSpy: SpyInstance;
+    let apiFetchSpy: MockInstance;
+    let configOnErrorSpy: MockInstance;
+    let checksumSpy: MockInstance;
     const config = getConfig();
     beforeEach(() => {
       vi.resetAllMocks();
@@ -32,7 +32,7 @@ describe('networkLayer', () => {
         .mockImplementation(() => 'c4ca4238a0b923820dcc509a6f75849b');
     });
 
-    it('should call apiFetch once', () => {
+    test('should call apiFetch once', () => {
       setConfig({
         platform: 'unknown',
         projectName: 'testProjectName',
@@ -52,7 +52,7 @@ describe('networkLayer', () => {
       });
     });
 
-    it('should not call apiFetch when there are no events', () => {
+    test('should not call apiFetch when there are no events', () => {
       const emptyEventsList: Event[] = [];
       setConfig({
         platform: 'unknown',
@@ -65,7 +65,7 @@ describe('networkLayer', () => {
       expect(checksumSpy).toHaveBeenCalledTimes(0);
     });
 
-    it('should not call apiFetch when identity.isOptOut is true', () => {
+    test('should not call apiFetch when identity.isOptOut is true', () => {
       const identity = getIdentity();
       identity.isOptOut = true;
       setConfig({
@@ -90,8 +90,8 @@ describe('networkLayer', () => {
       },
     ];
 
-    let apiFetchSpy: SpyInstance;
-    let configOnErrorSpy: SpyInstance;
+    let apiFetchSpy: MockInstance;
+    let configOnErrorSpy: MockInstance;
     const config = getConfig();
     beforeEach(() => {
       vi.resetAllMocks();
@@ -104,7 +104,7 @@ describe('networkLayer', () => {
         .mockImplementation(() => 'apiFetch');
     });
 
-    it('should call apiFetch when skipScheduler is set to true', async () => {
+    test('should call apiFetch when skipScheduler is set to true', async () => {
       setConfig({
         platform: 'unknown',
         projectName: 'testProjectName',
@@ -115,12 +115,12 @@ describe('networkLayer', () => {
       expect(configOnErrorSpy).toHaveBeenCalledTimes(0);
       expect(apiFetchSpy).toHaveBeenCalledWith({
         url: 'https://cca-lite.coinbase.com/metrics',
-        data: { metricData: metrics },
+        data: { metricData: JSON.stringify(metrics) },
         onError: config.onError,
       });
     });
 
-    it('should call apiFetch when skipScheduler is false', async () => {
+    test('should call apiFetch when skipScheduler is false', async () => {
       setConfig({
         platform: 'unknown',
         projectName: 'testProjectName',
@@ -131,14 +131,14 @@ describe('networkLayer', () => {
       expect(configOnErrorSpy).toHaveBeenCalledTimes(0);
       expect(apiFetchSpy).toHaveBeenCalledWith({
         url: 'https://cca-lite.coinbase.com/metrics',
-        data: { metricData: metrics },
+        data: { metricData: JSON.stringify(metrics) },
         onError: config.onError,
       });
     });
   });
 
   describe('networkLayerInit', () => {
-    it('should return a NetworkLayer object', () => {
+    test('should return a NetworkLayer object', () => {
       const networkLayer = createNetworkLayer();
 
       expect(networkLayer).toBeDefined();

src/storage/networkLayer.ts

@@ -5,7 +5,6 @@ import { NetworkLayer } from '../types/networkLayer';
 import { getNow } from '../utils/time';
 import { getChecksum } from '../utils/dataIntegrity';
 import { apiFetch } from '../utils/apiFetch';
-import { scheduleEvent } from './scheduler';
 
 const NO_OP = () => {};
 
@@ -14,6 +13,24 @@ export const DEFAULT_NETWORK_LAYER = {
   sendEvents: NO_OP,
 };
 
+
+/*
+ * Schedule an event
+ * - on web we create a background task with the requestIdleCallback API
+ * - on iOS and android we use the InteractionManager to schedule
+ *   a task after interactions or animations have completed,
+ *   this helps especially animations to run smoothly.
+ */
+// TODO: this should be moved to the scheduler
+export const scheduleEvent = (cb: () => void) => {
+  const config = getConfig();
+  if (window?.requestIdleCallback) {
+    window.requestIdleCallback(cb, { timeout: config.ricTimeoutScheduleEvent });
+  } else {
+    cb();
+  }
+};
+
 export const sendEvents = (events: Event[]) => {
   const identity = getIdentity();
   if (identity.isOptOut || events.length === 0) {

src/storage/scheduler.ts

@@ -1,6 +1,5 @@
 import { createQueue } from '../utils/queue';
 import { CreateScheduler, Scheduler } from '../types/scheduler';
-import { getConfig } from './storage';
 
 const DEFAULT_BATCH_THRESHOLD = 30;
 const DEFAULT_TIME_THRESHOLD = 5000;
@@ -57,18 +56,3 @@ export const createScheduler: CreateScheduler = <T>(
   };
 };
 
-/*
- * Schedule an event
- * - on web we create a background task with the requestIdleCallback API
- * - on iOS and android we use the InteractionManager to schedule
- *   a task after interactions or animations have completed,
- *   this helps especially animations to run smoothly.
- */
-export const scheduleEvent = (cb: () => void) => {
-  const config = getConfig();
-  if (window?.requestIdleCallback) {
-    window.requestIdleCallback(cb, { timeout: config.ricTimeoutScheduleEvent });
-  } else {
-    cb();
-  }
-};

src/utils/enhancers.test.ts

@@ -1,4 +1,4 @@
-import { describe, test, expect, beforeEach, vi } from 'vitest';
+import {describe, test, expect, beforeEach, vi, afterEach} from 'vitest';
 import {
   metricEnhancers,
   eventEnhancers,
@@ -370,12 +370,30 @@ describe('enhance', () => {
   });
 
   describe('device', () => {
+    let originalInnerHeight = 0;
+    let originalInnerWidth = 0;
+    beforeEach(() => {
+        const device = getDevice();
+        // reset device properties
+        Object.assign(device, { height: null, width: null });
+        // copy original window properties
+        originalInnerHeight = window.innerHeight;
+        originalInnerWidth = window.innerWidth;
+        //  set window properties
+        Object.defineProperty(window, 'innerHeight', { value: 1000 });
+        Object.defineProperty(window, 'innerWidth', { value: 800 });
+    });
+
+    afterEach(() => {
+      // reset window properties
+      Object.defineProperty(window, 'innerHeight', {value: originalInnerHeight});
+      Object.defineProperty(window, 'innerWidth', {value: originalInnerWidth});
+    });
+
     test('device enhancer sets device properties', () => {
       const device = getDevice();
-      Object.defineProperty(window, 'innerHeight', { value: 1000 });
-      Object.defineProperty(window, 'innerWidth', { value: 800 });
       deviceEnhancer();
-      expect(device).toContain({
+      expect(device).toMatchObject({
         height: 1000,
         width: 800,
       });

src/utils/enhancers.ts

@@ -34,13 +34,18 @@ const setDeviceSize = (properties: SetDeviceSize) => {
   device.width = properties.width;
 };
 
+// TODO: create a test for userAgent
+export const setUserAgent = () => {
+    const device = getDevice();
+    device.userAgent = window?.navigator?.userAgent || null;
+};
+
 /**
  * Set device information based on the platform used
  */
 // TODO: move to device
 export const setDevice = () => {
-  const device = getDevice();
-  device.userAgent = window?.navigator?.userAgent || null;
+  setUserAgent();
   setDeviceSize({
     height: window?.innerHeight ?? null,
     width: window?.innerWidth ?? null,

src/utils/perfume.test.ts

@@ -1,4 +1,4 @@
-import { describe, test, expect, vi, beforeEach, SpyInstance } from 'vitest';
+import { describe, test, expect, vi, beforeEach, MockInstance } from 'vitest';
 
 import { DEFAULT_CONFIG } from '../storage/config';
 import { init as storageInit } from '../storage/storage';
@@ -14,6 +14,7 @@ import {
   initPerfMonitoring,
   markNTBT,
 } from './perfume';
+import * as Perfume from 'perfume.js';
 
 import { getConfig } from '../storage/storage';
 import { PlatformName } from '../types/config';
@@ -43,9 +44,17 @@ const DEFAULT_TEST_STORAGE_CONFIG = {
   onError: () => undefined,
 };
 
+vi.mock('perfume.js', async () => {
+  const actual = await vi.importActual('perfume.js');
+  return {
+    ...actual,
+    markNTBT: vi.fn(),
+  }
+});
+
 describe('perfume', () => {
-  let trackEventSpy: SpyInstance;
-  let trackMetricSpy: SpyInstance;
+  let trackEventSpy: MockInstance;
+  let trackMetricSpy: MockInstance;
 
   describe('.getPerfumeOptions()', () => {
     let perfumeOptions;
@@ -601,18 +610,16 @@ describe('perfume', () => {
   });
 
   describe('perfumeInstance', () => {
-    const mockMarkNTBT = vi.fn();
-
     beforeEach(() => {
       storageInit({ ...DEFAULT_TEST_STORAGE_CONFIG, platform: 'web' });
     });
 
+    //  TODO: fix this test
     test('should call markNTBT() when platform is web', () => {
       initPerfMonitoring();
-      // eslint-disable-next-line @typescript-eslint/no-unused-expressions
-      // perfumeInstance ? (perfumeInstance.markNTBT = mockMarkNTBT) : null;
+      const markNTBTSpy = vi.spyOn(Perfume, 'markNTBT');
       markNTBT();
-      expect(mockMarkNTBT).toHaveBeenCalled();
+      expect(markNTBTSpy).toHaveBeenCalled();
     });
   });
 });