Skip to content

Commit 937f2d9

Browse files
shivamku-BSclaude
andcommitted
chore(o11y): trim the flush-guard comments to the non-obvious rationale
Comments outnumbered code roughly 2:1. Kept only what stops the boundary being undone by a later refactor -- why a throw here skips the rest of the spec, why cy.now rather than cy.task, why try/catch as well as .catch, and why console.warn rather than cy.task. No logic change (33 -> 12 comment lines). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 82cf3f9 commit 937f2d9

1 file changed

Lines changed: 14 additions & 35 deletions

File tree

bin/testObservability/cypress/index.js

Lines changed: 14 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -339,47 +339,28 @@ Cypress.Commands.add('fatal', (message, file) => {
339339
});
340340
});
341341

342-
/*
343-
* [SDK-7399] Drain eventsQueue without ever being able to fail the customer's suite.
344-
*
345-
* These flush sites run inside mocha `beforeEach`/`afterEach`. Up to v1.32.8 the same
346-
* events were dispatched from `Cypress.on(...)` listeners — i.e. OUTSIDE any mocha hook —
347-
* and every dispatch was additionally wrapped in `.catch()`, so an instrumentation
348-
* failure could not affect the run. v1.33.0 moved the dispatch INTO these hooks and
349-
* dropped all error handling, which makes instrumentation errors fatal to the customer:
350-
* a throw inside a hook fails that hook, and mocha then SKIPS EVERY REMAINING TEST in
351-
* the suite. That is the reported symptom — tests reported as skipped that the customer
352-
* never skipped.
353-
*
354-
* Two independent guards are required, because both failure modes were observed:
355-
* 1. SYNCHRONOUS throw — `cy.task`/`cy.now` can throw straight out of the call
356-
* (`TypeError: Cannot read properties of null (reading 'get')` raised inside
357-
* Cypress' own `runPrivilegedCommand`). A promise `.catch()` never runs for this,
358-
* so a real try/catch is needed.
359-
* 2. ASYNCHRONOUS rejection — handled by the promise `.catch()`.
360-
*
361-
* `cy.now('task', ...)` is used rather than `cy.task(...)`: `cy.task` enqueues a Cypress
362-
* command, so a failure surfaces later while the queue drains and fails the hook no
363-
* matter what guard wraps the enqueue call. `cy.now` executes immediately and returns a
364-
* promise, which is exactly what v1.32.8 did and is therefore containable here.
365-
*/
366-
/*
367-
* Every suppression below must leave a trace. A dropped event is invisible to the
368-
* customer's run by design, but it must not be invisible to us — otherwise a future
369-
* flush failure only shows up as data quietly missing from the dashboard. `console.warn`
370-
* is used deliberately rather than `browserStackLog`/`cy.task`: routing a diagnostic
371-
* through another Cypress command would reintroduce exactly the failure being contained.
372-
*/
342+
/* console.warn, not browserStackLog/cy.task — routing a diagnostic through another
343+
* Cypress command would reintroduce the failure this boundary contains. [SDK-7399] */
373344
const warnFlushFailure = (stage, err) => {
374345
try {
375346
console.warn(`BrowserStack Test Observability: suppressed ${stage} error, event(s) dropped: ${err && err.message ? err.message : err}`);
376347
} catch (e) { /* logging must never throw either */ }
377348
};
378349

350+
/*
351+
* [SDK-7399] These flush sites run inside mocha beforeEach/afterEach, so a throw here
352+
* fails the hook and mocha then SKIPS every remaining test in the spec. Before v1.33.0
353+
* the same events were dispatched from Cypress.on(...) listeners — outside any hook,
354+
* each wrapped in .catch() — so a failure was harmless. Keep this boundary intact:
355+
* - cy.now, not cy.task: cy.task enqueues, so its failure surfaces later during queue
356+
* drain and fails the hook regardless of any guard here. cy.now runs immediately.
357+
* - try/catch AND .catch: cy.now can throw synchronously out of Cypress'
358+
* runPrivilegedCommand, which a promise .catch() never sees.
359+
*/
379360
const flushEventsQueue = () => {
380361
try {
381362
const queued = eventsQueue;
382-
eventsQueue = [];
363+
eventsQueue = []; /* cleared before dispatch so a throw cannot replay these events */
383364
queued.forEach(event => {
384365
try {
385366
const payload = sanitizeForTask(event.data);
@@ -389,12 +370,10 @@ const flushEventsQueue = () => {
389370
result.catch(err => warnFlushFailure(`async dispatch of '${event.task}'`, err));
390371
}
391372
} catch (e) {
392-
/* one bad event must not stop the remaining events, and must not fail the hook */
393-
warnFlushFailure(`dispatch of '${event.task}'`, e);
373+
warnFlushFailure(`dispatch of '${event.task}'`, e); /* skip one event, not the rest */
394374
}
395375
});
396376
} catch (e) {
397-
/* instrumentation must never break the customer's test run */
398377
warnFlushFailure('queue flush', e);
399378
eventsQueue = [];
400379
}

0 commit comments

Comments
 (0)