From 57ce05056c392940568f69e89234dd57a71f929a Mon Sep 17 00:00:00 2001 From: Cal Courtney Date: Wed, 15 Apr 2026 16:29:06 +0100 Subject: [PATCH 1/4] refactor: use environment names instead of IDs for rule matching --- src/__tests__/context.test.js | 109 +++++++++++++++++++++------- src/__tests__/matcher.test.js | 103 ++++++++++---------------- src/context.ts | 132 ++++++++++++++++++---------------- src/matcher.ts | 4 +- 4 files changed, 191 insertions(+), 157 deletions(-) diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index bac8a02..8a450fe 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -2093,12 +2093,12 @@ describe("Context", () => { name: "Production Only", type: "assign", conditions: { and: [{ eq: [{ var: "country" }, { value: "US" }] }] }, - environments: [10], + environments: ["production"], variant: 1, }, ], }), - }, { environment_id: 10 }); + }); const rulesStrictContextResponse = buildRulesResponse({ audienceStrict: true, @@ -2132,30 +2132,19 @@ describe("Context", () => { expect(context.treatment("exp_test_abc")).toEqual(expectedVariants["exp_test_abc"]); }); - it("should skip rule when environment id does not match", () => { - const stagingResponse = { - ...envScopedRulesContextResponse, - environment_id: 20, - }; - const context = new Context(sdk, contextOptions, contextParams, stagingResponse); - context.attribute("country", "US"); - // Rule scoped to env 10, context has env 20 — should get normal assignment (2) - expect(context.treatment("exp_test_abc")).toEqual(expectedVariants["exp_test_abc"]); - }); - - it("should skip environment-scoped rules when API response has no environment_id", () => { - const noEnvIdResponse = { ...envScopedRulesContextResponse }; - delete noEnvIdResponse.environment_id; - const context = new Context(sdk, contextOptions, contextParams, noEnvIdResponse); + it("should skip rule when environment name does not match", () => { + client.getEnvironment.mockReturnValue("staging"); + const context = new Context(sdk, contextOptions, contextParams, envScopedRulesContextResponse); context.attribute("country", "US"); - // Rule requires env 10, but no environment_id in response — should get normal assignment + // Rule scoped to "production", client has "staging" — should get normal assignment (2) expect(context.treatment("exp_test_abc")).toEqual(expectedVariants["exp_test_abc"]); + client.getEnvironment.mockReturnValue("production"); }); - it("should match rule when environment id matches", () => { + it("should match rule when environment name matches", () => { const context = new Context(sdk, contextOptions, contextParams, envScopedRulesContextResponse); context.attribute("country", "US"); - // Rule scoped to env 10, context has env 10 — should get rule variant (1) + // Rule scoped to "production", client has "production" — should get rule variant (1) expect(context.treatment("exp_test_abc")).toEqual(1); }); @@ -2218,7 +2207,7 @@ describe("Context", () => { }); }); - it("should set correct flags when rule matches with audienceMismatch", (done) => { + it("should not evaluate audience when rule matches (audienceMismatch stays false)", (done) => { const context = new Context(sdk, contextOptions, contextParams, rulesStrictContextResponse); context.attribute("country", "US"); expect(context.treatment("exp_test_abc")).toEqual(1); @@ -2235,7 +2224,7 @@ describe("Context", () => { overridden: false, fullOn: false, custom: false, - audienceMismatch: true, + audienceMismatch: false, ruleOverride: true, }); done(); @@ -2407,7 +2396,7 @@ describe("Context", () => { expect(context.treatment("exp_test_abc")).toEqual(expectedVariants["exp_test_abc"]); }); - it("should match rule scoped to multiple environment ids", () => { + it("should match rule scoped to multiple environment names", () => { const multiEnvResponse = buildRulesResponse({ audience: JSON.stringify({ filter: [{ value: true }], @@ -2418,12 +2407,12 @@ describe("Context", () => { name: "Prod and Staging", type: "assign", conditions: { and: [{ eq: [{ var: "country" }, { value: "US" }] }] }, - environments: [10, 20], + environments: ["production", "staging"], variant: 1, }, ], }), - }, { environment_id: 20 }); + }); const context = new Context(sdk, contextOptions, contextParams, multiEnvResponse); context.attribute("country", "US"); expect(context.treatment("exp_test_abc")).toEqual(1); @@ -2559,6 +2548,70 @@ describe("Context", () => { expect(context.treatment("exp_test_abc")).toEqual(expectedVariants["exp_test_abc"]); }); + it("should match rule on application attribute when includeSystemAttributes is true", () => { + const appRuleResponse = buildRulesResponse({ + assignmentRules: JSON.stringify({ + rules: [ + { + name: "Website Only", + type: "assign", + conditions: { and: [{ eq: [{ var: "application" }, { value: "website" }] }] }, + environments: [], + variant: 1, + }, + ], + }), + }); + const optionsWithSystemAttrs = { + publishDelay: -1, + refreshPeriod: 0, + includeSystemAttributes: true, + }; + const context = new Context(sdk, optionsWithSystemAttrs, contextParams, appRuleResponse); + expect(context.treatment("exp_test_abc")).toEqual(1); + }); + + it("should not match rule on application attribute when includeSystemAttributes is not set", () => { + const appRuleResponse = buildRulesResponse({ + assignmentRules: JSON.stringify({ + rules: [ + { + name: "Website Only", + type: "assign", + conditions: { and: [{ eq: [{ var: "application" }, { value: "website" }] }] }, + environments: [], + variant: 1, + }, + ], + }), + }); + const context = new Context(sdk, contextOptions, contextParams, appRuleResponse); + expect(context.treatment("exp_test_abc")).toEqual(expectedVariants["exp_test_abc"]); + }); + + it("should match rule on environment attribute when includeSystemAttributes is true", () => { + const envRuleResponse = buildRulesResponse({ + assignmentRules: JSON.stringify({ + rules: [ + { + name: "Production Only", + type: "assign", + conditions: { and: [{ eq: [{ var: "environment" }, { value: "production" }] }] }, + environments: [], + variant: 1, + }, + ], + }), + }); + const optionsWithSystemAttrs = { + publishDelay: -1, + refreshPeriod: 0, + includeSystemAttributes: true, + }; + const context = new Context(sdk, optionsWithSystemAttrs, contextParams, envRuleResponse); + expect(context.treatment("exp_test_abc")).toEqual(1); + }); + it("should not invalidate cache when out-of-range rule variant changes to a different out-of-range value", () => { const oobRulesResponse = buildRulesResponse({ audience: JSON.stringify({ @@ -4658,7 +4711,7 @@ describe("Context", () => { }); it("should include app_version when application version is set", (done) => { - client.getApplication.mockReturnValueOnce({ name: "website", version: 3 }); + client.getApplication.mockReturnValue({ name: "website", version: 3 }); const optionsWithSystemAttrs = { publishDelay: -1, @@ -4679,6 +4732,7 @@ describe("Context", () => { expect(appVersionAttr).toBeDefined(); expect(appVersionAttr.value).toEqual(3); + client.getApplication.mockReturnValue({ name: "website", version: 0 }); done(); }); }); @@ -4707,7 +4761,7 @@ describe("Context", () => { }); it("should include app_version when application version is a semver string", (done) => { - client.getApplication.mockReturnValueOnce({ name: "website", version: "1.2.3" }); + client.getApplication.mockReturnValue({ name: "website", version: "1.2.3" }); const optionsWithSystemAttrs = { publishDelay: -1, @@ -4728,6 +4782,7 @@ describe("Context", () => { expect(appVersionAttr).toBeDefined(); expect(appVersionAttr.value).toEqual("1.2.3"); + client.getApplication.mockReturnValue({ name: "website", version: 0 }); done(); }); }); diff --git a/src/__tests__/matcher.test.js b/src/__tests__/matcher.test.js index f85b333..b0424e9 100644 --- a/src/__tests__/matcher.test.js +++ b/src/__tests__/matcher.test.js @@ -28,12 +28,12 @@ describe("AudienceMatcher", () => { }); describe("evaluateRules", () => { it("should return null when no rules in audience", () => { - expect(matcher.evaluateRules("{}", 1, {})).toBe(null); - expect(matcher.evaluateRules('{"filter":[]}', 1, {})).toBe(null); + expect(matcher.evaluateRules("{}", "production", {})).toBe(null); + expect(matcher.evaluateRules('{"filter":[]}', "production", {})).toBe(null); }); it("should return null when rules is empty array", () => { - expect(matcher.evaluateRules('{"rules":[]}', 1, {})).toBe(null); + expect(matcher.evaluateRules('{"rules":[]}', "production", {})).toBe(null); }); it("should return variant when conditions match", () => { @@ -48,7 +48,7 @@ describe("AudienceMatcher", () => { }, ], }); - expect(matcher.evaluateRules(audience, 1, { country: "US" })).toBe(1); + expect(matcher.evaluateRules(audience, "production", { country: "US" })).toBe(1); }); it("should return null when conditions do not match", () => { @@ -63,38 +63,38 @@ describe("AudienceMatcher", () => { }, ], }); - expect(matcher.evaluateRules(audience, 1, { country: "GB" })).toBe(null); + expect(matcher.evaluateRules(audience, "production", { country: "GB" })).toBe(null); }); - it("should skip rules with non-matching environment ids", () => { + it("should skip rules with non-matching environment names", () => { const audience = JSON.stringify({ rules: [ { name: "rule1", type: "assign", conditions: { and: [{ eq: [{ var: "country" }, { value: "US" }] }] }, - environments: [2], + environments: ["staging"], variant: 1, }, ], }); - expect(matcher.evaluateRules(audience, 1, { country: "US" })).toBe(null); + expect(matcher.evaluateRules(audience, "production", { country: "US" })).toBe(null); }); - it("should match when environment id is in the environments list", () => { + it("should match when environment name is in the environments list", () => { const audience = JSON.stringify({ rules: [ { name: "rule1", type: "assign", conditions: { and: [{ eq: [{ var: "country" }, { value: "US" }] }] }, - environments: [1, 2], + environments: ["production", "staging"], variant: 2, }, ], }); - expect(matcher.evaluateRules(audience, 1, { country: "US" })).toBe(2); - expect(matcher.evaluateRules(audience, 2, { country: "US" })).toBe(2); + expect(matcher.evaluateRules(audience, "production", { country: "US" })).toBe(2); + expect(matcher.evaluateRules(audience, "staging", { country: "US" })).toBe(2); }); it("should match all environments when environments is empty", () => { @@ -109,19 +109,19 @@ describe("AudienceMatcher", () => { }, ], }); - expect(matcher.evaluateRules(audience, 1, {})).toBe(1); - expect(matcher.evaluateRules(audience, 2, {})).toBe(1); + expect(matcher.evaluateRules(audience, "production", {})).toBe(1); + expect(matcher.evaluateRules(audience, "staging", {})).toBe(1); expect(matcher.evaluateRules(audience, null, {})).toBe(1); }); - it("should skip rules when environments is non-empty and environmentId is null", () => { + it("should skip rules when environments is non-empty and environment name is null", () => { const audience = JSON.stringify({ rules: [ { name: "rule1", type: "assign", conditions: { value: true }, - environments: [1], + environments: ["production"], variant: 1, }, ], @@ -148,7 +148,7 @@ describe("AudienceMatcher", () => { }, ], }); - expect(matcher.evaluateRules(audience, 1, { country: "US" })).toBe(1); + expect(matcher.evaluateRules(audience, "production", { country: "US" })).toBe(1); }); it("should return variant when conditions is null (matches all)", () => { @@ -163,7 +163,7 @@ describe("AudienceMatcher", () => { }, ], }); - expect(matcher.evaluateRules(audience, 1, {})).toBe(3); + expect(matcher.evaluateRules(audience, "production", {})).toBe(3); }); it("should return variant when conditions field is absent (matches all)", () => { @@ -177,12 +177,12 @@ describe("AudienceMatcher", () => { }, ], }); - expect(matcher.evaluateRules(audience, 1, {})).toBe(3); + expect(matcher.evaluateRules(audience, "production", {})).toBe(3); }); it("should handle malformed audience JSON gracefully", () => { - expect(matcher.evaluateRules("not json", 1, {})).toBe(null); - expect(matcher.evaluateRules("", 1, {})).toBe(null); + expect(matcher.evaluateRules("not json", "production", {})).toBe(null); + expect(matcher.evaluateRules("", "production", {})).toBe(null); }); it("should return null when rule has no variant property", () => { @@ -195,7 +195,7 @@ describe("AudienceMatcher", () => { }, ], }); - expect(matcher.evaluateRules(audience, 1, {})).toBe(null); + expect(matcher.evaluateRules(audience, "production", {})).toBe(null); }); it("should return null when variant is not a number", () => { @@ -209,7 +209,7 @@ describe("AudienceMatcher", () => { }, ], }); - expect(matcher.evaluateRules(audience, 1, {})).toBe(null); + expect(matcher.evaluateRules(audience, "production", {})).toBe(null); }); it("should skip rule with invalid variant and continue to next valid rule", () => { @@ -229,7 +229,7 @@ describe("AudienceMatcher", () => { }, ], }); - expect(matcher.evaluateRules(audience, 1, {})).toBe(2); + expect(matcher.evaluateRules(audience, "production", {})).toBe(2); }); it("should skip rule with missing variant and continue to next valid rule", () => { @@ -248,12 +248,12 @@ describe("AudienceMatcher", () => { }, ], }); - expect(matcher.evaluateRules(audience, 1, {})).toBe(1); + expect(matcher.evaluateRules(audience, "production", {})).toBe(1); }); it("should handle malformed rules gracefully", () => { - expect(matcher.evaluateRules('{"rules":"not an array"}', 1, {})).toBe(null); - expect(matcher.evaluateRules('{"rules":[null]}', 1, {})).toBe(null); + expect(matcher.evaluateRules('{"rules":"not an array"}', "production", {})).toBe(null); + expect(matcher.evaluateRules('{"rules":[null]}', "production", {})).toBe(null); }); it("should skip rules with non-assign type", () => { @@ -273,7 +273,7 @@ describe("AudienceMatcher", () => { }, ], }); - expect(matcher.evaluateRules(audience, 1, {})).toBe(2); + expect(matcher.evaluateRules(audience, "production", {})).toBe(2); }); it("should skip rules with missing type", () => { @@ -286,7 +286,7 @@ describe("AudienceMatcher", () => { }, ], }); - expect(matcher.evaluateRules(audience, 1, {})).toBe(null); + expect(matcher.evaluateRules(audience, "production", {})).toBe(null); }); it("should skip to second rule when first does not match", () => { @@ -308,7 +308,7 @@ describe("AudienceMatcher", () => { }, ], }); - expect(matcher.evaluateRules(audience, 1, { country: "US" })).toBe(2); + expect(matcher.evaluateRules(audience, "production", { country: "US" })).toBe(2); }); it("should support variant 0", () => { @@ -323,7 +323,7 @@ describe("AudienceMatcher", () => { }, ], }); - expect(matcher.evaluateRules(audience, 1, {})).toBe(0); + expect(matcher.evaluateRules(audience, "production", {})).toBe(0); }); it("should skip rule with fractional variant", () => { @@ -343,7 +343,7 @@ describe("AudienceMatcher", () => { }, ], }); - expect(matcher.evaluateRules(audience, 1, {})).toBe(2); + expect(matcher.evaluateRules(audience, "production", {})).toBe(2); }); it("should skip rule with non-object conditions", () => { @@ -364,7 +364,7 @@ describe("AudienceMatcher", () => { }, ], }); - expect(matcher.evaluateRules(audience, 1, {})).toBe(2); + expect(matcher.evaluateRules(audience, "production", {})).toBe(2); }); it("should skip rule when environments is not an array", () => { @@ -379,38 +379,7 @@ describe("AudienceMatcher", () => { }, ], }); - expect(matcher.evaluateRules(audience, 1, {})).toBe(null); - }); - - it("should not match integer environmentIds against fractional entries in environments list", () => { - const audience = JSON.stringify({ - rules: [ - { - name: "rule1", - type: "assign", - conditions: { value: true }, - environments: [1.5], - variant: 1, - }, - ], - }); - expect(matcher.evaluateRules(audience, 1, {})).toBe(null); - expect(matcher.evaluateRules(audience, 2, {})).toBe(null); - }); - - it("should skip environment-scoped rule when environmentId is 0 and not in list", () => { - const audience = JSON.stringify({ - rules: [ - { - name: "rule1", - type: "assign", - conditions: { value: true }, - environments: [1, 2], - variant: 1, - }, - ], - }); - expect(matcher.evaluateRules(audience, 0, {})).toBe(null); + expect(matcher.evaluateRules(audience, "production", {})).toBe(null); }); it("should skip rule when conditions evaluation throws and continue to next rule", () => { @@ -431,7 +400,7 @@ describe("AudienceMatcher", () => { }, ], }); - expect(matcher.evaluateRules(audience, 1, {})).toBe(2); + expect(matcher.evaluateRules(audience, "production", {})).toBe(2); }); it("should return negative variant (bounds checking is caller responsibility)", () => { @@ -446,7 +415,7 @@ describe("AudienceMatcher", () => { }, ], }); - expect(matcher.evaluateRules(audience, 1, {})).toBe(-1); + expect(matcher.evaluateRules(audience, "production", {})).toBe(-1); }); }); }); diff --git a/src/context.ts b/src/context.ts index 404e2e3..2962dae 100644 --- a/src/context.ts +++ b/src/context.ts @@ -126,7 +126,6 @@ export type ContextOptions = { export type ContextData = { experiments?: ExperimentData[]; - environment_id?: number; }; export default class Context { @@ -135,7 +134,7 @@ export default class Context { private readonly _audienceMatcher: AudienceMatcher; private readonly _cassignments: Record; private readonly _dataProvider: ContextDataProvider; - private _environmentId: number | null; + private _environmentName: string | null; private readonly _eventLogger: EventLogger; private readonly _opts: ContextOptions; private readonly _publisher: ContextPublisher; @@ -175,7 +174,7 @@ export default class Context { this._units = {}; this._assigners = {}; this._audienceMatcher = new AudienceMatcher(); - this._environmentId = null; + this._environmentName = null; this._attrsSeq = 0; if (params.units) { @@ -441,7 +440,7 @@ export default class Context { } private _computeRuleVariant(assignmentRules: string, variantCount: number, attrs: Record): number | null { - const rawRuleVariant = this._audienceMatcher.evaluateRules(assignmentRules, this._environmentId, attrs); + const rawRuleVariant = this._audienceMatcher.evaluateRules(assignmentRules, this._environmentName, attrs); return rawRuleVariant !== null && rawRuleVariant >= 0 && rawRuleVariant < variantCount ? rawRuleVariant : null; @@ -459,6 +458,15 @@ export default class Context { private _getAttributesMap(): Record { const attrs: Record = {}; + if (this._opts.includeSystemAttributes === true) { + const client = this._sdk.getClient(); + const app = client.getApplication(); + attrs["application"] = app.name; + attrs["environment"] = client.getEnvironment(); + if ((typeof app.version === "string" && app.version.length > 0) || (typeof app.version === "number" && app.version > 0)) { + attrs["app_version"] = app.version; + } + } this._attrs.forEach((attr) => { attrs[attr.name] = attr.value; }); @@ -478,7 +486,7 @@ export default class Context { const audienceMatches = (experiment: ExperimentData, assignment: Assignment) => { const ruleKey = experiment.assignmentRules - ? `${experiment.assignmentRules}:${this._environmentId}` + ? `${experiment.assignmentRules}:${this._environmentName}` : ""; const ruleKeyChanged = ruleKey !== (assignment.ruleKey ?? ""); @@ -494,15 +502,6 @@ export default class Context { if (this._attrsSeq > (assignment.attrsSeq ?? 0) || ruleKeyChanged) { const attrs = this._getAttributesMap(); - if (experiment.audience && experiment.audience.length > 0) { - const result = this._audienceMatcher.evaluate(experiment.audience, attrs); - const newAudienceMismatch = typeof result === "boolean" ? !result : false; - - if (newAudienceMismatch !== assignment.audienceMismatch) { - return false; - } - } - if (experiment.assignmentRules && experiment.assignmentRules.length > 0) { const ruleVariant = this._computeRuleVariant(experiment.assignmentRules, experiment.variants.length, attrs); if (ruleVariant !== (assignment.ruleVariant ?? null)) { @@ -512,6 +511,15 @@ export default class Context { assignment.ruleVariant = ruleVariant; } + if (!assignment.ruleOverride && experiment.audience && experiment.audience.length > 0) { + const result = this._audienceMatcher.evaluate(experiment.audience, attrs); + const newAudienceMismatch = typeof result === "boolean" ? !result : false; + + if (newAudienceMismatch !== assignment.audienceMismatch) { + return false; + } + } + assignment.ruleKey = ruleKey; assignment.attrsSeq = this._attrsSeq; } @@ -575,69 +583,71 @@ export default class Context { let ruleVariant: number | null = null; const attrs = this._getAttributesMap(); - if (experiment.data.audience && experiment.data.audience.length > 0) { - const result = this._audienceMatcher.evaluate(experiment.data.audience, attrs); - - if (typeof result === "boolean") { - assignment.audienceMismatch = !result; - } - } - if (experiment.data.assignmentRules && experiment.data.assignmentRules.length > 0) { ruleVariant = this._computeRuleVariant(experiment.data.assignmentRules, experiment.data.variants.length, attrs); } assignment.ruleVariant = ruleVariant; assignment.ruleKey = experiment.data.assignmentRules - ? `${experiment.data.assignmentRules}:${this._environmentId}` + ? `${experiment.data.assignmentRules}:${this._environmentName}` : ""; if (ruleVariant !== null && ruleVariant >= 0 && ruleVariant < experiment.data.variants.length) { assignment.variant = ruleVariant; assignment.ruleOverride = true; - } else if (experiment.data.audienceStrict && assignment.audienceMismatch) { - assignment.variant = 0; - } else if (experiment.data.fullOnVariant === 0) { - if (unitType !== null) { - if (unitType in this._units) { - const unit = this._unitHash(unitType); - if (unit !== null) { - const assigner = - unitType in this._assigners - ? this._assigners[unitType] - : (this._assigners[unitType] = new VariantAssigner(unit)); - const eligible = - assigner.assign( - experiment.data.trafficSplit, - experiment.data.trafficSeedHi, - experiment.data.trafficSeedLo - ) === 1; - - assignment.assigned = true; - assignment.eligible = eligible; - - if (eligible) { - if (hasCustom) { - assignment.variant = this._cassignments[experimentName]; - assignment.custom = true; + } else { + if (experiment.data.audience && experiment.data.audience.length > 0) { + const result = this._audienceMatcher.evaluate(experiment.data.audience, attrs); + + if (typeof result === "boolean") { + assignment.audienceMismatch = !result; + } + } + + if (experiment.data.audienceStrict && assignment.audienceMismatch) { + assignment.variant = 0; + } else if (experiment.data.fullOnVariant === 0) { + if (unitType !== null) { + if (unitType in this._units) { + const unit = this._unitHash(unitType); + if (unit !== null) { + const assigner = + unitType in this._assigners + ? this._assigners[unitType] + : (this._assigners[unitType] = new VariantAssigner(unit)); + const eligible = + assigner.assign( + experiment.data.trafficSplit, + experiment.data.trafficSeedHi, + experiment.data.trafficSeedLo + ) === 1; + + assignment.assigned = true; + assignment.eligible = eligible; + + if (eligible) { + if (hasCustom) { + assignment.variant = this._cassignments[experimentName]; + assignment.custom = true; + } else { + assignment.variant = assigner.assign( + experiment.data.split, + experiment.data.seedHi, + experiment.data.seedLo + ); + } } else { - assignment.variant = assigner.assign( - experiment.data.split, - experiment.data.seedHi, - experiment.data.seedLo - ); + assignment.variant = 0; } - } else { - assignment.variant = 0; } } } + } else { + assignment.assigned = true; + assignment.eligible = true; + assignment.variant = experiment.data.fullOnVariant; + assignment.fullOn = true; } - } else { - assignment.assigned = true; - assignment.eligible = true; - assignment.variant = experiment.data.fullOnVariant; - assignment.fullOn = true; } // store these so we can detect changes to running experiment @@ -1013,7 +1023,7 @@ export default class Context { private _init(data: ContextData, assignments: Record = {}) { this._data = data; - this._environmentId = data.environment_id ?? null; + this._environmentName = this._sdk.getClient().getEnvironment() ?? null; const index: Record = {}; const indexVariables: Record = {}; diff --git a/src/matcher.ts b/src/matcher.ts index 3b1a993..31c0c01 100644 --- a/src/matcher.ts +++ b/src/matcher.ts @@ -17,7 +17,7 @@ export class AudienceMatcher { return null; } - evaluateRules(assignmentRulesString: string, environmentId: number | null, vars: Record): number | null { + evaluateRules(assignmentRulesString: string, environmentName: string | null, vars: Record): number | null { let assignmentRules; try { assignmentRules = JSON.parse(assignmentRulesString); @@ -37,7 +37,7 @@ export class AudienceMatcher { if (!Array.isArray(rule.environments)) continue; if (rule.environments.length > 0) { - if (environmentId == null || !rule.environments.includes(environmentId)) { + if (environmentName == null || !rule.environments.includes(environmentName)) { continue; } } From 99a4a7fd986a5fb9494a736a009ee7a3f75d5255 Mon Sep 17 00:00:00 2001 From: Cal Courtney Date: Wed, 15 Apr 2026 16:41:50 +0100 Subject: [PATCH 2/4] chore: full-review comments --- src/__tests__/context.test.js | 32 ++++++++++++++++++++++++++++---- src/__tests__/matcher.test.js | 15 +++++++++++++++ src/context.ts | 2 +- 3 files changed, 44 insertions(+), 5 deletions(-) diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index 8a450fe..f0573e2 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -2055,9 +2055,8 @@ describe("Context", () => { }); describe("rules evaluation", () => { - const buildRulesResponse = (overrides = {}, responseOverrides = {}) => ({ + const buildRulesResponse = (overrides = {}) => ({ ...getContextResponse, - ...responseOverrides, experiments: getContextResponse.experiments.map((x) => { if (x.name === "exp_test_abc") { return { ...x, ...overrides }; @@ -2133,12 +2132,18 @@ describe("Context", () => { }); it("should skip rule when environment name does not match", () => { - client.getEnvironment.mockReturnValue("staging"); + client.getEnvironment.mockReturnValueOnce("staging"); const context = new Context(sdk, contextOptions, contextParams, envScopedRulesContextResponse); context.attribute("country", "US"); // Rule scoped to "production", client has "staging" — should get normal assignment (2) expect(context.treatment("exp_test_abc")).toEqual(expectedVariants["exp_test_abc"]); - client.getEnvironment.mockReturnValue("production"); + }); + + it("should skip environment-scoped rules when getEnvironment returns undefined", () => { + client.getEnvironment.mockReturnValueOnce(undefined); + const context = new Context(sdk, contextOptions, contextParams, envScopedRulesContextResponse); + context.attribute("country", "US"); + expect(context.treatment("exp_test_abc")).toEqual(expectedVariants["exp_test_abc"]); }); it("should match rule when environment name matches", () => { @@ -2322,6 +2327,25 @@ describe("Context", () => { expect(context.treatment("exp_test_abc")).toEqual(0); }); + it("should set audienceMismatch true when no rule matches and audience mismatches in strict mode", (done) => { + const context = new Context(sdk, contextOptions, contextParams, rulesStrictContextResponse); + context.attribute("country", "GB"); + expect(context.treatment("exp_test_abc")).toEqual(0); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + const publishCall = publisher.publish.mock.calls[0][0]; + const exposure = publishCall.exposures.find((e) => e.name === "exp_test_abc"); + expect(exposure).toMatchObject({ + variant: 0, + ruleOverride: false, + audienceMismatch: true, + }); + done(); + }); + }); + it("should return correct variableValue when rule forces a different variant", () => { const context = new Context(sdk, contextOptions, contextParams, rulesContextResponse); context.attribute("country", "US"); diff --git a/src/__tests__/matcher.test.js b/src/__tests__/matcher.test.js index b0424e9..ecc52d4 100644 --- a/src/__tests__/matcher.test.js +++ b/src/__tests__/matcher.test.js @@ -382,6 +382,21 @@ describe("AudienceMatcher", () => { expect(matcher.evaluateRules(audience, "production", {})).toBe(null); }); + it("should be case-sensitive when matching environment names", () => { + const audience = JSON.stringify({ + rules: [ + { + name: "rule1", + type: "assign", + conditions: { value: true }, + environments: ["Production"], + variant: 1, + }, + ], + }); + expect(matcher.evaluateRules(audience, "production", {})).toBe(null); + }); + it("should skip rule when conditions evaluation throws and continue to next rule", () => { const audience = JSON.stringify({ rules: [ diff --git a/src/context.ts b/src/context.ts index 2962dae..85c2459 100644 --- a/src/context.ts +++ b/src/context.ts @@ -592,7 +592,7 @@ export default class Context { ? `${experiment.data.assignmentRules}:${this._environmentName}` : ""; - if (ruleVariant !== null && ruleVariant >= 0 && ruleVariant < experiment.data.variants.length) { + if (ruleVariant !== null) { assignment.variant = ruleVariant; assignment.ruleOverride = true; } else { From 138e8658f1d67be3ba4324051e9306a17e6d09c2 Mon Sep 17 00:00:00 2001 From: Cal Courtney Date: Wed, 15 Apr 2026 16:51:45 +0100 Subject: [PATCH 3/4] fix: normalize environment attribute to null instead of undefined --- src/context.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/context.ts b/src/context.ts index 85c2459..3ac366b 100644 --- a/src/context.ts +++ b/src/context.ts @@ -462,7 +462,7 @@ export default class Context { const client = this._sdk.getClient(); const app = client.getApplication(); attrs["application"] = app.name; - attrs["environment"] = client.getEnvironment(); + attrs["environment"] = client.getEnvironment() ?? null; if ((typeof app.version === "string" && app.version.length > 0) || (typeof app.version === "number" && app.version > 0)) { attrs["app_version"] = app.version; } @@ -866,7 +866,7 @@ export default class Context { { name: "sdk_name", value: client.getAgent(), setAt: now }, { name: "sdk_version", value: SDK_VERSION, setAt: now }, { name: "application", value: app.name, setAt: now }, - { name: "environment", value: client.getEnvironment(), setAt: now } + { name: "environment", value: client.getEnvironment() ?? null, setAt: now } ); if ((typeof app.version === "string" && app.version.length > 0) || (typeof app.version === "number" && app.version > 0)) { allAttributes.push({ name: "app_version", value: app.version, setAt: now }); From 2ced41cdf2a2651a0966c5244f4e418d45a7cf5b Mon Sep 17 00:00:00 2001 From: Cal Courtney Date: Wed, 15 Apr 2026 17:23:28 +0100 Subject: [PATCH 4/4] refactor: move mock cleanup to afterEach in app_version tests --- src/__tests__/context.test.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index f0573e2..2ec6de0 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -4641,6 +4641,10 @@ describe("Context", () => { }); describe("includeSystemAttributes", () => { + afterEach(() => { + client.getApplication.mockReturnValue({ name: "website", version: 0 }); + }); + it("should not include system attributes by default", (done) => { const defaultOptions = { publishDelay: -1, @@ -4756,7 +4760,6 @@ describe("Context", () => { expect(appVersionAttr).toBeDefined(); expect(appVersionAttr.value).toEqual(3); - client.getApplication.mockReturnValue({ name: "website", version: 0 }); done(); }); }); @@ -4806,7 +4809,6 @@ describe("Context", () => { expect(appVersionAttr).toBeDefined(); expect(appVersionAttr.value).toEqual("1.2.3"); - client.getApplication.mockReturnValue({ name: "website", version: 0 }); done(); }); });