Skip to content

Commit a8d5051

Browse files
edenoedeno
committed
refactor(tests): address code review feedback
Implements all 4 suggestions from Copilot PR review: 1. Move API URL constants to module-level scope (schema-contracts.test.js) - Extracted TRODES_SCHEMA_URL and PROBE_METADATA_API_URL to module level - Improves reusability and follows established pattern 2. Document setTimeout rationale (integration-test-helpers.js) - Clarified that waitFor + setTimeout serve different purposes - waitFor ensures input cleared, setTimeout ensures React state updates complete - Needed for rapid successive updates in loop 3. Add memory leak fix reference (test-hooks.js) - Added detailed comment explaining memory leak fix - Includes commit reference: 83ca8c6 - Helps future maintainers understand context 4. Extract duplicated validation wrapper (DRY principle) - Created src/__tests__/helpers/baseline-compatibility.js - Extracted jsonschemaValidation() and rulesValidation() wrappers - Removed duplication from validation.baseline.test.js and performance.baseline.test.js - Improves consistency and reduces technical debt All tests passing: 2074/2074 ✓
1 parent 3edeb61 commit a8d5051

File tree

6 files changed

+113
-101
lines changed

6 files changed

+113
-101
lines changed

‎src/__tests__/baselines/performance.baseline.test.js‎

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -13,23 +13,11 @@
1313
import { describe, it, expect } from 'vitest';
1414
import { renderWithProviders } from '../helpers/test-utils';
1515
import App from '../../App';
16-
import { validate } from '../../validation';
16+
import { jsonschemaValidation } from '../helpers/baseline-compatibility';
1717
import fs from 'fs';
1818
import path from 'path';
1919
import yaml from 'yaml';
2020

21-
/**
22-
* Compatibility wrapper: converts new validate() API to old jsonschemaValidation() format
23-
*/
24-
function jsonschemaValidation(formContent) {
25-
const issues = validate(formContent);
26-
const schemaIssues = issues.filter(issue => issue.instancePath !== undefined);
27-
return {
28-
valid: schemaIssues.length === 0,
29-
errors: schemaIssues.length === 0 ? null : schemaIssues,
30-
};
31-
}
32-
3321
// Load test fixtures for realistic performance testing
3422
const fixturesPath = path.join(__dirname, '../fixtures/valid');
3523
const minimalYaml = yaml.parse(

‎src/__tests__/baselines/validation.baseline.test.js‎

Lines changed: 1 addition & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import { describe, it, expect } from 'vitest';
1414
import YAML from 'yaml';
1515
import fs from 'fs';
1616
import path from 'path';
17-
import { validate } from '../../validation';
17+
import { jsonschemaValidation, rulesValidation } from '../helpers/baseline-compatibility';
1818

1919
/**
2020
* Helper to load YAML fixtures
@@ -30,88 +30,6 @@ function loadFixture(category, filename) {
3030
return YAML.parse(content);
3131
}
3232

33-
/**
34-
* Compatibility wrapper: converts new validate() API to old jsonschemaValidation() format
35-
* This allows baseline tests to continue working without modification
36-
*/
37-
function jsonschemaValidation(formContent) {
38-
const issues = validate(formContent);
39-
40-
// Filter to only schema validation issues (have instancePath)
41-
const schemaIssues = issues.filter(issue => issue.instancePath !== undefined);
42-
43-
const isValid = schemaIssues.length === 0;
44-
45-
const validationMessages = schemaIssues.map((issue) => {
46-
return `Key: ${issue.instancePath
47-
.split('/')
48-
.filter((x) => x !== '')
49-
.join(', ')}. | Error: ${issue.message}`;
50-
});
51-
52-
const errorIds = [
53-
...new Set(
54-
schemaIssues.map((issue) => {
55-
const validationEntries = issue.instancePath
56-
.split('/')
57-
.filter((x) => x !== '');
58-
return validationEntries[0];
59-
})
60-
),
61-
];
62-
63-
// Convert to AJV error format for compatibility
64-
const errors = schemaIssues.length === 0 ? null : schemaIssues.map(issue => ({
65-
instancePath: issue.instancePath,
66-
schemaPath: issue.schemaPath,
67-
message: issue.message,
68-
}));
69-
70-
return {
71-
valid: isValid,
72-
isValid,
73-
jsonSchemaErrorMessages: validationMessages,
74-
jsonSchemaErrors: errors,
75-
jsonSchemaErrorIds: errorIds,
76-
errors,
77-
};
78-
}
79-
80-
/**
81-
* Compatibility wrapper: converts new validate() API to old rulesValidation() format
82-
*/
83-
function rulesValidation(jsonFileContent) {
84-
const issues = validate(jsonFileContent);
85-
86-
// Filter to only rules validation issues (no instancePath)
87-
const rulesIssues = issues.filter(issue => issue.instancePath === undefined);
88-
89-
const isFormValid = rulesIssues.length === 0;
90-
91-
const errorMessages = rulesIssues.map(issue => issue.message);
92-
93-
const errorIds = [
94-
...new Set(
95-
rulesIssues.map(issue => {
96-
const topLevelField = issue.path.split('[')[0].split('.')[0];
97-
return topLevelField;
98-
})
99-
)
100-
];
101-
102-
const errors = rulesIssues.map(issue => ({
103-
id: issue.path.replace(/\[(\d+)\]\.(\w+)/g, '-$2-$1').replace(/\[(\d+)\]$/g, '-$1'),
104-
message: issue.message,
105-
}));
106-
107-
return {
108-
isFormValid,
109-
formErrors: errorMessages,
110-
formErrorIds: errorIds,
111-
errors,
112-
};
113-
}
114-
11533
describe('BASELINE: JSON Schema Validation', () => {
11634
describe('Valid YAML Acceptance', () => {
11735
it('accepts minimal valid YAML with all required fields', () => {

‎src/__tests__/helpers/baseline-compatibility.js‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
/**
2+
* Baseline Test Compatibility Wrappers
3+
*
4+
* These functions convert the new unified validate() API back to the old
5+
* separate jsonschemaValidation() and rulesValidation() formats.
6+
*
7+
* This allows baseline tests to continue working without modification during refactoring.
8+
*
9+
* DO NOT use these in new tests - use validate() directly instead.
10+
*/
11+
12+
import { validate } from '../../validation';
13+
14+
/**
15+
* Compatibility wrapper: converts new validate() API to old jsonschemaValidation() format
16+
* This allows baseline tests to continue working without modification
17+
*
18+
* @param {Object} formContent - Form data to validate
19+
* @returns {Object} Validation result in old format
20+
*/
21+
export function jsonschemaValidation(formContent) {
22+
const issues = validate(formContent);
23+
24+
// Filter to only schema validation issues (have instancePath)
25+
const schemaIssues = issues.filter(issue => issue.instancePath !== undefined);
26+
27+
const isValid = schemaIssues.length === 0;
28+
29+
const validationMessages = schemaIssues.map((issue) => {
30+
return `Key: ${issue.instancePath
31+
.split('/')
32+
.filter((x) => x !== '')
33+
.join(', ')}. | Error: ${issue.message}`;
34+
});
35+
36+
const errorIds = [
37+
...new Set(
38+
schemaIssues.map((issue) => {
39+
const validationEntries = issue.instancePath
40+
.split('/')
41+
.filter((x) => x !== '');
42+
return validationEntries[0];
43+
})
44+
),
45+
];
46+
47+
// Convert to AJV error format for compatibility
48+
const errors = schemaIssues.length === 0 ? null : schemaIssues.map(issue => ({
49+
instancePath: issue.instancePath,
50+
schemaPath: issue.schemaPath,
51+
message: issue.message,
52+
}));
53+
54+
return {
55+
valid: isValid,
56+
isValid,
57+
jsonSchemaErrorMessages: validationMessages,
58+
jsonSchemaErrors: errors,
59+
jsonSchemaErrorIds: errorIds,
60+
errors,
61+
};
62+
}
63+
64+
/**
65+
* Compatibility wrapper: converts new validate() API to old rulesValidation() format
66+
*
67+
* @param {Object} jsonFileContent - Form data to validate
68+
* @returns {Object} Validation result in old format
69+
*/
70+
export function rulesValidation(jsonFileContent) {
71+
const issues = validate(jsonFileContent);
72+
73+
// Filter to only rules validation issues (no instancePath)
74+
const rulesIssues = issues.filter(issue => issue.instancePath === undefined);
75+
76+
const isFormValid = rulesIssues.length === 0;
77+
78+
const errorMessages = rulesIssues.map(issue => issue.message);
79+
80+
const errorIds = [
81+
...new Set(
82+
rulesIssues.map(issue => {
83+
const topLevelField = issue.path.split('[')[0].split('.')[0];
84+
return topLevelField;
85+
})
86+
)
87+
];
88+
89+
const errors = rulesIssues.map(issue => ({
90+
id: issue.path.replace(/\[(\d+)\]\.(\w+)/g, '-$2-$1').replace(/\[(\d+)\]$/g, '-$1'),
91+
message: issue.message,
92+
}));
93+
94+
return {
95+
isFormValid,
96+
formErrors: errorMessages,
97+
formErrorIds: errorIds,
98+
errors,
99+
};
100+
}

‎src/__tests__/helpers/integration-test-helpers.js‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,9 @@ export async function addTask(user, screen, task) {
218218
await waitFor(() => {
219219
expect(taskEpochInput.value).toBe('');
220220
});
221-
// Small delay to ensure state update completes
221+
// Additional delay needed for rapid successive updates in loop
222+
// waitFor ensures input is cleared, setTimeout ensures React state updates
223+
// (taskEpochsDefined dependency) complete before next iteration
222224
await new Promise(resolve => setTimeout(resolve, 100));
223225
}
224226

‎src/__tests__/helpers/test-hooks.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,10 @@ export function useCreateElementMock(beforeEachFn, afterEachFn) {
164164
* NOTE: Previously named useWebkitURLMock - renamed to use standard URL API
165165
* instead of vendor-prefixed webkitURL (P0.1 memory leak fix)
166166
*
167+
* Memory Leak Fix: Blob URLs created during YAML export were not being revoked,
168+
* causing memory leaks when generating many files. Now properly revoked after download.
169+
* See commit: 83ca8c6 "fix(yaml): prevent memory leak by revoking blob URLs after download"
170+
*
167171
* @param {Function} beforeEachFn - Vitest beforeEach function
168172
* @param {Function} afterEachFn - Vitest afterEach function
169173
* @param {string} mockURL - Mock URL to return (default: 'blob:mock-url')

‎src/__tests__/integration/schema-contracts.test.js‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,10 @@ import crypto from 'crypto';
1919
import schema from '../../nwb_schema.json';
2020
import { deviceTypes } from '../../valueList';
2121

22+
// API URLs for integration contract verification
23+
const TRODES_SCHEMA_URL = 'https://raw.githubusercontent.com/LorenFrankLab/trodes_to_nwb/main/src/trodes_to_nwb/nwb_schema.json';
24+
const PROBE_METADATA_API_URL = 'https://api.github.com/repos/LorenFrankLab/trodes_to_nwb/contents/src/trodes_to_nwb/device_metadata/probe_metadata';
25+
2226
describe('BASELINE: Integration Contracts', () => {
2327
describe('Schema Hash', () => {
2428
it('documents current schema version', () => {
@@ -35,8 +39,6 @@ describe('BASELINE: Integration Contracts', () => {
3539
// Fetch schema from GitHub main branch to verify synchronization
3640
// This works in any environment (local, CI, contributor machines)
3741

38-
const TRODES_SCHEMA_URL = 'https://raw.githubusercontent.com/LorenFrankLab/trodes_to_nwb/main/src/trodes_to_nwb/nwb_schema.json';
39-
4042
try {
4143
const response = await fetch(TRODES_SCHEMA_URL);
4244

@@ -105,8 +107,6 @@ describe('BASELINE: Integration Contracts', () => {
105107
// Fetch probe metadata directory from GitHub to verify device types exist
106108
// This works in any environment (local, CI, contributor machines)
107109

108-
const PROBE_METADATA_API_URL = 'https://api.github.com/repos/LorenFrankLab/trodes_to_nwb/contents/src/trodes_to_nwb/device_metadata/probe_metadata';
109-
110110
try {
111111
const response = await fetch(PROBE_METADATA_API_URL);
112112

0 commit comments

Comments
 (0)