Skip to content

Commit 3346eb9

Browse files
JosephSamirLclaude
JosephSamirL
and
claude
committed
fix(context,tests): address third-round review and SonarCloud duplication
Two remaining items from the Gemini review on the prior commit: #3 — `runVariation` Visual Editor check used `c?.data?.['js']` bracket access while the rest of the body uses dot access (`data.css`, `data.js`, `data.custom_js`). Align via a narrow structural cast, `(c as {data?: {js?: string}})?.data?.js`. No behavior change. #4 — `experienceId` falls back to `'unknown'` when both `experience.id` and `bucketedVariation.experienceId` are missing, so DOM marker IDs never interpolate the string `'undefined'` (which would collide with any other change whose ids were also undefined). Degenerate-config guard; should never trigger with a valid config. SonarCloud duplication on new code — the new mocha tests in `data-manager.tests.ts` and `experience-manager.tests.ts` each repeated the same `server.on('request', …)` boilerplate 4-5 times, pushing new-code duplication to 4.9% (gate is ≤3%). Extracted an `awaitTrackRequest(server, accountId, projectId, done, assertFn)` helper in each file and refactored only the NEW blocks added by this PR. Pre-PR blocks left untouched to keep the diff minimal. `yarn lint --fix` additionally removed three now-unused `eslint-disable-next-line mocha/no-hooks-for-single-case` directives on hook blocks that gained a second test case during this PR. Tests: mocha 352/352 passing locally, Playwright 45/45 passing (including 9 runVariation tests). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent f419112 commit 3346eb9

3 files changed

Lines changed: 113 additions & 116 deletions

File tree

‎packages/data/tests/data-manager.tests.ts‎

Lines changed: 60 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,33 @@ const release_timeout = 1000;
3434
const test_timeout = release_timeout + 1000;
3535
const batch_size = 10;
3636

37+
// Test helper: register a one-shot handler that runs `assertFn` once the
38+
// SDK's tracking request arrives, then resolves the mocha `done`. Replaces
39+
// the inline `server.on('request', ...)` boilerplate that otherwise gets
40+
// repeated in every async test (and trips SonarCloud's duplication gate).
41+
function awaitTrackRequest(
42+
server: http.Server,
43+
trackAccountId: string,
44+
trackProjectId: string,
45+
done: Mocha.Done,
46+
assertFn: () => void
47+
): void {
48+
server.on('request', (request, res) => {
49+
if (request.url.startsWith(`/track/${trackAccountId}/${trackProjectId}`)) {
50+
request.on('end', () => {
51+
try {
52+
assertFn();
53+
done();
54+
} catch (err) {
55+
done(err);
56+
}
57+
});
58+
}
59+
res.writeHead(200, {'Content-Type': 'application/json'});
60+
res.end('{}');
61+
});
62+
}
63+
3764
const configuration = objectDeepMerge(testConfig, defaultConfig, {
3865
api: {
3966
endpoint: {
@@ -71,7 +98,7 @@ describe('DataManager tests', function () {
7198
customSegments: ['seg1', 'seg2']
7299
};
73100
let dataManager, accountId, projectId, storeKey, server;
74-
// eslint-disable-next-line mocha/no-hooks-for-single-case
101+
75102
before(function () {
76103
accountId = configuration?.data?.account_id;
77104
projectId = configuration?.data?.project?.id;
@@ -83,12 +110,12 @@ describe('DataManager tests', function () {
83110
apiManager
84111
});
85112
});
86-
// eslint-disable-next-line mocha/no-hooks-for-single-case
113+
87114
beforeEach(function () {
88115
server = http.createServer();
89116
server.listen(port);
90117
});
91-
// eslint-disable-next-line mocha/no-hooks-for-single-case
118+
92119
afterEach(function () {
93120
dataManager.reset();
94121
server.closeAllConnections();
@@ -307,7 +334,6 @@ describe('DataManager tests', function () {
307334
});
308335
});
309336
describe('Persistent Data Store enqueue tests', function () {
310-
// eslint-disable-next-line mocha/no-hooks-for-single-case
311337
before(function () {
312338
configuration.dataStore = dataStore;
313339
dataManager = new dm(
@@ -357,7 +383,6 @@ describe('DataManager tests', function () {
357383
});
358384
});
359385
describe('Persistent Data Store tests (set immediately)', function () {
360-
// eslint-disable-next-line mocha/no-hooks-for-single-case
361386
before(function () {
362387
dataStore.data = {};
363388
delete configuration.dataStore;
@@ -401,16 +426,15 @@ describe('DataManager tests', function () {
401426
describe('Test ruleDataProvider integration', function () {
402427
let receivedRuleData;
403428
let provider;
404-
// eslint-disable-next-line mocha/no-hooks-for-single-case
429+
405430
before(function () {
406431
// Mock RuleManager that captures what data shape was passed to isRuleMatched
407432
const capturingRuleManager: any = {
408433
isRuleMatched: (data) => {
409434
receivedRuleData = data;
410435
return true;
411436
},
412-
isUsingCustomInterface: (data) =>
413-
!!data && data.name === 'RuleData'
437+
isUsingCustomInterface: (data) => !!data && data.name === 'RuleData'
414438
};
415439
provider = {
416440
name: 'RuleData',
@@ -438,18 +462,11 @@ describe('DataManager tests', function () {
438462
dataManager.getBucketing(visitorId, experienceKey, {
439463
locationProperties: {url: 'https://convert.com/'}
440464
});
441-
server.on('request', (request, res) => {
442-
if (request.url.startsWith(`/track/${accountId}/${projectId}`)) {
443-
request.on('end', () => {
444-
expect(receivedRuleData)
445-
.to.be.an('object')
446-
.that.has.property('name', 'RuleData');
447-
expect(receivedRuleData).to.equal(provider);
448-
done();
449-
});
450-
}
451-
res.writeHead(200, {'Content-Type': 'application/json'});
452-
res.end('{}');
465+
awaitTrackRequest(server, accountId, projectId, done, () => {
466+
expect(receivedRuleData)
467+
.to.be.an('object')
468+
.that.has.property('name', 'RuleData');
469+
expect(receivedRuleData).to.equal(provider);
453470
});
454471
});
455472
it('Should let per-call visitorProperties win over a configured ruleDataProvider', function (done) {
@@ -462,17 +479,10 @@ describe('DataManager tests', function () {
462479
visitorProperties: {varName3: 'plain-value'},
463480
locationProperties: {url: 'https://convert.com/'}
464481
});
465-
server.on('request', (request, res) => {
466-
if (request.url.startsWith(`/track/${accountId}/${projectId}`)) {
467-
request.on('end', () => {
468-
expect(receivedRuleData).to.be.an('object');
469-
expect(receivedRuleData).to.not.have.property('name', 'RuleData');
470-
expect(receivedRuleData).to.have.property('varName3', 'plain-value');
471-
done();
472-
});
473-
}
474-
res.writeHead(200, {'Content-Type': 'application/json'});
475-
res.end('{}');
482+
awaitTrackRequest(server, accountId, projectId, done, () => {
483+
expect(receivedRuleData).to.be.an('object');
484+
expect(receivedRuleData).to.not.have.property('name', 'RuleData');
485+
expect(receivedRuleData).to.have.property('varName3', 'plain-value');
476486
});
477487
});
478488
it('Should fall back to plain visitorProperties when no ruleDataProvider is set', function (done) {
@@ -486,8 +496,7 @@ describe('DataManager tests', function () {
486496
receivedRuleData = data;
487497
return true;
488498
},
489-
isUsingCustomInterface: (data) =>
490-
!!data && data.name === 'RuleData'
499+
isUsingCustomInterface: (data) => !!data && data.name === 'RuleData'
491500
};
492501
const localDataManager = new dm(noProviderConfig, {
493502
bucketingManager,
@@ -500,18 +509,11 @@ describe('DataManager tests', function () {
500509
visitorProperties: {varName3: 'plain-value'},
501510
locationProperties: {url: 'https://convert.com/'}
502511
});
503-
server.on('request', (request, res) => {
504-
if (request.url.startsWith(`/track/${accountId}/${projectId}`)) {
505-
request.on('end', () => {
506-
// No provider configured — RuleManager should see the plain object.
507-
expect(receivedRuleData).to.be.an('object');
508-
expect(receivedRuleData).to.not.have.property('name', 'RuleData');
509-
expect(receivedRuleData).to.have.property('varName3', 'plain-value');
510-
done();
511-
});
512-
}
513-
res.writeHead(200, {'Content-Type': 'application/json'});
514-
res.end('{}');
512+
awaitTrackRequest(server, accountId, projectId, done, () => {
513+
// No provider configured — RuleManager should see the plain object.
514+
expect(receivedRuleData).to.be.an('object');
515+
expect(receivedRuleData).to.not.have.property('name', 'RuleData');
516+
expect(receivedRuleData).to.have.property('varName3', 'plain-value');
515517
});
516518
});
517519
it('Should warn and ignore a misconfigured ruleDataProvider (missing name)', function () {
@@ -564,20 +566,17 @@ describe('DataManager tests', function () {
564566
type: 'event'
565567
// intentionally no `rules` field
566568
};
567-
const configWithProvider = objectDeepMerge(
568-
configuration,
569-
{
570-
ruleDataProvider: {
571-
name: 'RuleData',
572-
getUrl: () => 'https://convert.com/'
573-
},
574-
dataStore: undefined,
575-
data: {
576-
...configuration.data,
577-
goals: [...configuration.data.goals, ruleLessGoal]
578-
}
569+
const configWithProvider = objectDeepMerge(configuration, {
570+
ruleDataProvider: {
571+
name: 'RuleData',
572+
getUrl: () => 'https://convert.com/'
573+
},
574+
dataStore: undefined,
575+
data: {
576+
...configuration.data,
577+
goals: [...configuration.data.goals, ruleLessGoal]
579578
}
580-
) as unknown as ConfigType;
579+
}) as unknown as ConfigType;
581580
const localDataManager = new dm(configWithProvider, {
582581
bucketingManager,
583582
ruleManager,
@@ -591,16 +590,9 @@ describe('DataManager tests', function () {
591590
undefined,
592591
{}
593592
);
594-
server.on('request', (request, res) => {
595-
if (request.url.startsWith(`/track/${accountId}/${projectId}`)) {
596-
request.on('end', () => {
597-
// convert() returns `true` when the conversion fires
598-
expect(triggered).to.equal(true);
599-
done();
600-
});
601-
}
602-
res.writeHead(200, {'Content-Type': 'application/json'});
603-
res.end('{}');
593+
awaitTrackRequest(server, accountId, projectId, done, () => {
594+
// convert() returns `true` when the conversion fires
595+
expect(triggered).to.equal(true);
604596
});
605597
});
606598
});

‎packages/experience/tests/experience-manager.tests.ts‎

Lines changed: 45 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,33 @@ const release_timeout = 1000;
2121
const test_timeout = release_timeout + 1000;
2222
const batch_size = 5;
2323

24+
// Test helper: register a one-shot handler that runs `assertFn` once the
25+
// SDK's tracking request arrives, then resolves the mocha `done`. Replaces
26+
// the inline `server.on('request', ...)` boilerplate that otherwise gets
27+
// repeated in every async test (and trips SonarCloud's duplication gate).
28+
function awaitTrackRequest(
29+
server: http.Server,
30+
trackAccountId: string,
31+
trackProjectId: string,
32+
done: Mocha.Done,
33+
assertFn: () => void
34+
): void {
35+
server.on('request', (request, res) => {
36+
if (request.url.startsWith(`/track/${trackAccountId}/${trackProjectId}`)) {
37+
request.on('end', () => {
38+
try {
39+
assertFn();
40+
done();
41+
} catch (err) {
42+
done(err);
43+
}
44+
});
45+
}
46+
res.writeHead(200, {'Content-Type': 'application/json'});
47+
res.end('{}');
48+
});
49+
}
50+
2451
const configuration = objectDeepMerge(testConfig, defaultConfig, {
2552
api: {
2653
endpoint: {
@@ -41,7 +68,7 @@ const apiManager = new am(configuration, {eventManager});
4168
describe('ExperienceManager tests', function () {
4269
const visitorId = 'XXX';
4370
let dataManager, experienceManager, accountId, projectId, server;
44-
// eslint-disable-next-line mocha/no-hooks-for-single-case
71+
4572
before(function () {
4673
accountId = configuration?.data?.account_id;
4774
projectId = configuration?.data?.project?.id;
@@ -53,12 +80,12 @@ describe('ExperienceManager tests', function () {
5380
});
5481
experienceManager = new exm(configuration, {dataManager});
5582
});
56-
// eslint-disable-next-line mocha/no-hooks-for-single-case
83+
5784
beforeEach(function () {
5885
server = http.createServer();
5986
server.listen(port);
6087
});
61-
// eslint-disable-next-line mocha/no-hooks-for-single-case
88+
6289
afterEach(function () {
6390
dataManager.reset();
6491
server.closeAllConnections();
@@ -232,18 +259,11 @@ describe('ExperienceManager tests', function () {
232259
locationProperties: {url: 'https://convert.com/'}
233260
}
234261
);
235-
server.on('request', (request, res) => {
236-
if (request.url.startsWith(`/track/${accountId}/${projectId}`)) {
237-
request.on('end', () => {
238-
expect(variation)
239-
.to.be.an('object')
240-
.that.has.property('experienceType');
241-
expect(variation.experienceType).to.equal('a/b_fullstack');
242-
done();
243-
});
244-
}
245-
res.writeHead(200, {'Content-Type': 'application/json'});
246-
res.end('{}');
262+
awaitTrackRequest(server, accountId, projectId, done, () => {
263+
expect(variation)
264+
.to.be.an('object')
265+
.that.has.property('experienceType');
266+
expect(variation.experienceType).to.equal('a/b_fullstack');
247267
});
248268
});
249269
});
@@ -254,15 +274,8 @@ describe('ExperienceManager tests', function () {
254274
visitorProperties: {varName3: 'something'},
255275
locationProperties: {url: 'https://convert.com/'}
256276
});
257-
server.on('request', (request, res) => {
258-
if (request.url.startsWith(`/track/${accountId}/${projectId}`)) {
259-
request.on('end', () => {
260-
expect(variations).to.be.an('array').that.has.length(2);
261-
done();
262-
});
263-
}
264-
res.writeHead(200, {'Content-Type': 'application/json'});
265-
res.end('{}');
277+
awaitTrackRequest(server, accountId, projectId, done, () => {
278+
expect(variations).to.be.an('array').that.has.length(2);
266279
});
267280
});
268281
it('Should keep all fullstack experiences when experienceTypes=["a/b_fullstack"]', function (done) {
@@ -272,18 +285,11 @@ describe('ExperienceManager tests', function () {
272285
locationProperties: {url: 'https://convert.com/'},
273286
experienceTypes: ['a/b_fullstack']
274287
});
275-
server.on('request', (request, res) => {
276-
if (request.url.startsWith(`/track/${accountId}/${projectId}`)) {
277-
request.on('end', () => {
278-
expect(variations).to.be.an('array').that.has.length(2);
279-
variations.forEach((v) => {
280-
expect(v.experienceType).to.equal('a/b_fullstack');
281-
});
282-
done();
283-
});
284-
}
285-
res.writeHead(200, {'Content-Type': 'application/json'});
286-
res.end('{}');
288+
awaitTrackRequest(server, accountId, projectId, done, () => {
289+
expect(variations).to.be.an('array').that.has.length(2);
290+
variations.forEach((v) => {
291+
expect(v.experienceType).to.equal('a/b_fullstack');
292+
});
287293
});
288294
});
289295
it('Should return empty when filtering by a type with no matching experiences', function () {
@@ -337,16 +343,9 @@ describe('ExperienceManager tests', function () {
337343
experienceTypes: ['a/b_fullstack']
338344
}
339345
);
340-
server.on('request', (request, res) => {
341-
if (request.url.startsWith(`/track/${accountId}/${projectId}`)) {
342-
request.on('end', () => {
343-
expect(variation).to.be.an('object');
344-
expect(variation).to.have.property('experienceType', 'a/b_fullstack');
345-
done();
346-
});
347-
}
348-
res.writeHead(200, {'Content-Type': 'application/json'});
349-
res.end('{}');
346+
awaitTrackRequest(server, accountId, projectId, done, () => {
347+
expect(variation).to.be.an('object');
348+
expect(variation).to.have.property('experienceType', 'a/b_fullstack');
350349
});
351350
});
352351
});

‎packages/js-sdk/src/context.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -429,12 +429,18 @@ export class Context implements ContextInterface {
429429
);
430430
}
431431

432-
const experienceId = experience?.id ?? bucketedVariation.experienceId;
432+
// Fallback to 'unknown' if both ids are missing so marker IDs never
433+
// interpolate `undefined` (which would collide with any other change
434+
// that also produced an undefined id).
435+
const experienceId =
436+
experience?.id ?? bucketedVariation.experienceId ?? 'unknown';
433437

434438
if (
435439
!(window as any).convert?.T &&
436440
Array.isArray(bucketedVariation.changes) &&
437-
bucketedVariation.changes.some((c) => c?.data?.['js'])
441+
bucketedVariation.changes.some(
442+
(c) => (c as {data?: {js?: string}})?.data?.js
443+
)
438444
) {
439445
this._loggerManager?.warn?.(
440446
'Context.runVariation()',

0 commit comments

Comments
 (0)