Skip to content

Commit 1a88995

Browse files
fix(observability,session-status): don't mark suppressNotFoundErrors commands as test failures (#53)
* fix(observability): don't mark suppressNotFoundErrors commands as test failures Nightwatch records every isVisible / isPresent timeout with status:'fail' internally, including calls where the caller explicitly opted into "not found is fine" via suppressNotFoundErrors:true. sendTestRunEvent and the setSessionStatus path in globals.js both scanned commands[] for any status:'fail' and flipped the test/session to "failed" without checking the opt-out or the testcase envelope's pass/fail rollup, so tests Nightwatch and Automate considered passed were reported as failed in Test Observability. - Add helper.isSuppressedFailure(cmd) — handles args[0] as object or JSON string. - sendTestRunEvent: filter suppressed-failure commands and trust the envelope rollup (status:'pass' && failed:0 && errors:0) over a stray command-level fail status. - globals.js setSessionStatus: same fix, keeps Automate session status consistent with TRA. - Add 4 regression tests for sendTestRunEvent (previously uncovered). Linked: BrowserStack SDK-5914 * fix(observability): lint, tighten regression tests, and broaden helper coverage - src/utils/helper.js: satisfy padding-line-between-statements; log a debug message when args[0] is an unparseable JSON string; clarify in the doc-comment why only args[0] is inspected. - nightwatch/globals.js: clarify the eventData===null fallback behavior matches the pre-fix code (status defaults to 'passed'). - test/src/test-observability/sendTestRunEvent.js: tighten tests 3 & 4 to lock failure_reason content and backtrace shape — exactly the wire fields the original bug malformed. - test/src/utils/helper.js: add 10 unit tests for isSuppressedFailure covering null/undefined/primitive args[0], malformed JSON strings, object/string opt-out shapes, and falsy variants. Both call sites (sendTestRunEvent and setSessionStatus) are covered by reference.
1 parent 2dbe174 commit 1a88995

5 files changed

Lines changed: 273 additions & 5 deletions

File tree

‎nightwatch/globals.js‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -307,12 +307,22 @@ module.exports = {
307307
try {
308308
const testName = test?.testcase;
309309
const eventData = (testName && test?.envelope?.[testName]?.testcase) || null;
310+
// Skip commands the caller opted out of via suppressNotFoundErrors:true
311+
// and trust Nightwatch's envelope-level rollup over a stray command-level
312+
// fail status. Same defect shape as testObservability.js sendTestRunEvent.
313+
// When eventData itself is missing (session aborted before TestRunStarted
314+
// populated the envelope), failedCommand stays null and status defaults
315+
// to 'passed' — matches the pre-fix behaviour for that edge case.
310316
const failedCommand = eventData?.commands && Array.isArray(eventData.commands)
311-
? eventData.commands.find(cmd => cmd.status === 'fail')
317+
? eventData.commands.find(cmd => cmd.status === 'fail' && !helper.isSuppressedFailure(cmd))
312318
: null;
313-
const status = failedCommand ? 'failed' : 'passed';
319+
const envelopePassed = !!eventData
320+
&& (eventData.status === 'pass')
321+
&& ((eventData.failed || 0) === 0)
322+
&& ((eventData.errors || 0) === 0);
323+
const status = (failedCommand && !envelopePassed) ? 'failed' : 'passed';
314324
let reason = '';
315-
if (failedCommand && failedCommand.result) {
325+
if (status === 'failed' && failedCommand && failedCommand.result) {
316326
reason = (failedCommand.result.message || failedCommand.result.stack || 'Test failed').toString().slice(0, 280);
317327
}
318328
const payload = JSON.stringify({status, reason});

‎src/testObservability.js‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -537,8 +537,12 @@ class TestObservability {
537537
testData.finished_at = eventData.endTimestamp ? new Date(eventData.endTimestamp).toISOString() : new Date(startTimestamp).toISOString();
538538
testData.result = 'passed';
539539
if (eventData && eventData.commands && Array.isArray(eventData.commands)) {
540-
const failedCommand = eventData.commands.find(cmd => cmd.status === 'fail');
541-
if (failedCommand) {
540+
const failedCommand = eventData.commands.find(cmd => cmd.status === 'fail' && !helper.isSuppressedFailure(cmd));
541+
// Envelope-level rollup: when Nightwatch itself reports the testcase
542+
// as passed (no failed assertions / errors), trust the rollup over a
543+
// stray command-level fail status that did not propagate.
544+
const envelopePassed = (eventData.status === 'pass') && ((eventData.failed || 0) === 0) && ((eventData.errors || 0) === 0);
545+
if (failedCommand && !envelopePassed) {
542546
testData.result = 'failed';
543547
if (failedCommand.result) {
544548
testData.failure = [

‎src/utils/helper.js‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,30 @@ exports.isUndefined = value => (value === undefined || value === null);
6262

6363
exports.isObject = value => (!this.isUndefined(value) && value.constructor === Object);
6464

65+
// A Nightwatch command is recorded with status:'fail' even when the caller
66+
// explicitly opted into "not found is fine" via `suppressNotFoundErrors:true`.
67+
// Those commands carry no failure semantics for the test and must not flip
68+
// test/session status. Only `args[0]` is inspected — Nightwatch's reporter
69+
// always serializes element-command options as the first positional argument,
70+
// and either as an object literal or as a JSON-encoded string depending on
71+
// the reporter path. Both shapes must be accepted.
72+
exports.isSuppressedFailure = (cmd) => {
73+
if (!cmd || cmd.status !== 'fail' || !Array.isArray(cmd.args) || cmd.args.length === 0) {return false}
74+
const first = cmd.args[0];
75+
let opts = first;
76+
if (typeof first === 'string') {
77+
try {
78+
opts = JSON.parse(first);
79+
} catch (e) {
80+
Logger.debug(`isSuppressedFailure: could not parse args[0] for cmd ${cmd.name || '<unnamed>'}: ${e.message}`);
81+
82+
return false;
83+
}
84+
}
85+
86+
return !!(opts && typeof opts === 'object' && opts.suppressNotFoundErrors === true);
87+
};
88+
6589
exports.isTestObservabilitySession = () => {
6690
return process.env.BROWSERSTACK_TEST_OBSERVABILITY === 'true' ||
6791
process.env.BROWSERSTACK_TEST_REPORTING === 'true';
Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,164 @@
1+
const assert = require('assert');
2+
const sinon = require('sinon');
3+
4+
const helper = require('../../../src/utils/helper');
5+
const TestMap = require('../../../src/utils/testMap');
6+
const TestObservability = require('../../../src/testObservability');
7+
8+
// Regression coverage for SDK-5914 / suppressNotFoundErrors.
9+
//
10+
// Before this fix, sendTestRunEvent walked eventData.commands for any
11+
// status:'fail' record and flipped testData.result to 'failed' even when
12+
// the failing command was a Nightwatch `isVisible({suppressNotFoundErrors:true})`
13+
// lookup whose absence is an expected, callsite-opted-into outcome.
14+
// The bug shipped because this function had no unit coverage at all.
15+
describe('TestObservability - sendTestRunEvent (suppressNotFoundErrors)', function () {
16+
const buildTest = (commands, envelopeRollup = {status: 'pass', failed: 0, errors: 0}) => ({
17+
metadata: {
18+
name: 'Conditional Suite',
19+
tags: [],
20+
modulePath: '/tmp/observabilityBugRepro.js',
21+
host: 'hub-cloud.browserstack.com',
22+
sessionId: 'session-id-stub',
23+
sessionCapabilities: {}
24+
},
25+
testcase: 'conditional test',
26+
testCaseData: () => '',
27+
settings: {desiredCapabilities: {'bstack:options': {osVersion: '11'}}},
28+
envelope: {
29+
'conditional test': {
30+
startTimestamp: 1700000000000,
31+
testcase: {
32+
endTimestamp: 1700000001000,
33+
commands,
34+
...envelopeRollup
35+
}
36+
}
37+
}
38+
});
39+
40+
beforeEach(() => {
41+
this.sandbox = sinon.createSandbox();
42+
this.testObservability = new TestObservability();
43+
44+
this.sandbox.stub(this.testObservability, 'getTestBody').returns('');
45+
this.sandbox.stub(this.testObservability, 'processTestRunData').resolves();
46+
this.sandbox.stub(helper, 'getCloudProvider').returns('automate');
47+
this.sandbox.stub(helper, 'getIntegrationsObject').returns({});
48+
this.sandbox.stub(helper, 'isTestObservabilitySession').returns(true);
49+
this.sandbox.stub(helper, 'isAccessibilitySession').returns(false);
50+
this.sandbox.stub(TestMap, 'getSessionSnapshot').returns(null);
51+
52+
this.uploaded = null;
53+
this.uploadStub = this.sandbox.stub(helper, 'uploadEventData').callsFake(async (payload) => {
54+
this.uploaded = payload;
55+
});
56+
});
57+
58+
afterEach(() => {
59+
this.sandbox.restore();
60+
});
61+
62+
it('marks the test passed when the only failing command opted into suppressNotFoundErrors (args as object)', async () => {
63+
const commands = [
64+
{name: 'url', args: ['https://www.google.com'], status: 'pass'},
65+
{
66+
name: 'isVisible',
67+
args: [{selector: '#may-or-may-not-exist', suppressNotFoundErrors: true, timeout: 2000}, null],
68+
status: 'fail',
69+
result: {message: 'Element not found', stack: '', name: 'Error'}
70+
}
71+
];
72+
73+
await this.testObservability.sendTestRunEvent('TestRunFinished', buildTest(commands), 'uuid-1');
74+
75+
sinon.assert.calledOnce(this.uploadStub);
76+
assert.strictEqual(this.uploaded.event_type, 'TestRunFinished');
77+
assert.strictEqual(this.uploaded.test_run.result, 'passed');
78+
assert.ok(!('failure' in this.uploaded.test_run), 'expected no failure field on passed test');
79+
assert.ok(!('failure_reason' in this.uploaded.test_run), 'expected no failure_reason field on passed test');
80+
});
81+
82+
it('marks the test passed when args[0] is a JSON-encoded string carrying suppressNotFoundErrors', async () => {
83+
// Some Nightwatch reporter paths serialize the options object to a JSON
84+
// string in command.args[0] — the customer's CHROME_148__observabilityBugRepro.json
85+
// is the canonical example. The fix must handle both shapes.
86+
const commands = [
87+
{
88+
name: 'isVisible',
89+
args: ['{"selector":"#may-or-may-not-exist","suppressNotFoundErrors":true,"timeout":2000}', null],
90+
status: 'fail',
91+
result: {message: 'Element not found', stack: ''}
92+
}
93+
];
94+
95+
await this.testObservability.sendTestRunEvent('TestRunFinished', buildTest(commands), 'uuid-2');
96+
97+
assert.strictEqual(this.uploaded.test_run.result, 'passed');
98+
});
99+
100+
it('still marks the test failed when a real assertion failure is present', async () => {
101+
// Envelope rollup says failed:1 — a real failure happened. The fix must
102+
// NOT suppress that. This is the contrast case that prevents the patch
103+
// from silently downgrading every failing test to passed.
104+
const commands = [
105+
{
106+
name: 'assert.titleContains',
107+
args: ['Google'],
108+
status: 'fail',
109+
result: {message: 'Expected title to contain "Google"', stack: 'AssertionError', name: 'AssertionError'}
110+
}
111+
];
112+
113+
await this.testObservability.sendTestRunEvent(
114+
'TestRunFinished',
115+
buildTest(commands, {status: 'fail', failed: 1, errors: 0}),
116+
'uuid-3'
117+
);
118+
119+
assert.strictEqual(this.uploaded.test_run.result, 'failed');
120+
assert.strictEqual(this.uploaded.test_run.failure_type, 'AssertionError');
121+
// Lock the wire-shape that the original bug malformed (failure_reason:null,
122+
// backtrace:["",""]). The patch must propagate the real failure detail.
123+
assert.strictEqual(this.uploaded.test_run.failure_reason, 'Expected title to contain "Google"');
124+
assert.deepStrictEqual(
125+
this.uploaded.test_run.failure[0].backtrace,
126+
['Expected title to contain "Google"', 'AssertionError']
127+
);
128+
});
129+
130+
it('still marks the test failed when a non-suppressed command failed alongside a suppressed one', async () => {
131+
// Mixed case: one suppressed isVisible + one real failure. Envelope rollup
132+
// disagrees with "all passed", so we must propagate the real failure.
133+
const commands = [
134+
{
135+
name: 'isVisible',
136+
args: [{selector: '#optional', suppressNotFoundErrors: true}, null],
137+
status: 'fail',
138+
result: {message: 'Element not found'}
139+
},
140+
{
141+
name: 'click',
142+
args: ['#mandatory'],
143+
status: 'fail',
144+
result: {message: 'Element #mandatory not found', stack: 'NoSuchElementError', name: 'NoSuchElementError'}
145+
}
146+
];
147+
148+
await this.testObservability.sendTestRunEvent(
149+
'TestRunFinished',
150+
buildTest(commands, {status: 'fail', failed: 0, errors: 1}),
151+
'uuid-4'
152+
);
153+
154+
assert.strictEqual(this.uploaded.test_run.result, 'failed');
155+
// failedCommand must skip the suppressed one and pick the real one — confirm
156+
// by asserting the failure_reason references the mandatory selector, not the
157+
// suppressed optional one.
158+
assert.strictEqual(this.uploaded.test_run.failure_type, 'UnhandledError');
159+
assert.ok(
160+
this.uploaded.test_run.failure_reason.includes('#mandatory'),
161+
`expected failure_reason to reference the real failure, got: ${this.uploaded.test_run.failure_reason}`
162+
);
163+
});
164+
});

‎test/src/utils/helper.js‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -353,3 +353,69 @@ describe('isBrowserstackInfra', () => {
353353
});
354354

355355
});
356+
357+
describe('isSuppressedFailure', () => {
358+
// Shared primitive used by both src/testObservability.js (sendTestRunEvent)
359+
// and nightwatch/globals.js (setSessionStatus). Covers both call sites by
360+
// reference — any regression here breaks both surfaces equivalently.
361+
let isSuppressedFailure;
362+
before(() => {
363+
isSuppressedFailure = require('../../../src/utils/helper').isSuppressedFailure;
364+
});
365+
366+
it('returns false for null / undefined commands', () => {
367+
expect(isSuppressedFailure(null)).to.be.false;
368+
expect(isSuppressedFailure(undefined)).to.be.false;
369+
});
370+
371+
it('returns false for commands that did not fail', () => {
372+
expect(isSuppressedFailure({status: 'pass', args: [{suppressNotFoundErrors: true}]})).to.be.false;
373+
});
374+
375+
it('returns false when args is missing or empty', () => {
376+
expect(isSuppressedFailure({status: 'fail'})).to.be.false;
377+
expect(isSuppressedFailure({status: 'fail', args: []})).to.be.false;
378+
});
379+
380+
it('returns false when args is not an array', () => {
381+
expect(isSuppressedFailure({status: 'fail', args: {suppressNotFoundErrors: true}})).to.be.false;
382+
expect(isSuppressedFailure({status: 'fail', args: 'not-an-array'})).to.be.false;
383+
});
384+
385+
it('returns false when args[0] is a primitive', () => {
386+
expect(isSuppressedFailure({status: 'fail', args: [null]})).to.be.false;
387+
expect(isSuppressedFailure({status: 'fail', args: [42]})).to.be.false;
388+
expect(isSuppressedFailure({status: 'fail', args: [true]})).to.be.false;
389+
});
390+
391+
it('returns true when args[0] is an object with suppressNotFoundErrors:true', () => {
392+
expect(isSuppressedFailure({
393+
status: 'fail',
394+
args: [{selector: '#x', suppressNotFoundErrors: true, timeout: 2000}, null]
395+
})).to.be.true;
396+
});
397+
398+
it('returns true when args[0] is a JSON string carrying suppressNotFoundErrors:true', () => {
399+
expect(isSuppressedFailure({
400+
status: 'fail',
401+
args: ['{"selector":"#x","suppressNotFoundErrors":true,"timeout":2000}', null]
402+
})).to.be.true;
403+
});
404+
405+
it('returns false when args[0] is a malformed JSON string (safe default)', () => {
406+
// Unparseable strings default to "not suppressed" — i.e., the test stays
407+
// failed. That's the safer direction, matches the rest of the guard's
408+
// bias to under-suppress rather than over-suppress.
409+
expect(isSuppressedFailure({status: 'fail', args: ['{this is not json']})).to.be.false;
410+
});
411+
412+
it('returns false when args[0] is an object without suppressNotFoundErrors', () => {
413+
expect(isSuppressedFailure({status: 'fail', args: [{selector: '#x', timeout: 2000}]})).to.be.false;
414+
});
415+
416+
it('returns false when suppressNotFoundErrors is falsy', () => {
417+
expect(isSuppressedFailure({status: 'fail', args: [{suppressNotFoundErrors: false}]})).to.be.false;
418+
expect(isSuppressedFailure({status: 'fail', args: [{suppressNotFoundErrors: 'true'}]})).to.be.false;
419+
});
420+
421+
});

0 commit comments

Comments
 (0)