-
Notifications
You must be signed in to change notification settings - Fork 45
fix(observability): inject TRA support-file listener synchronously (SDK-7121) #1160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
osho-20
wants to merge
4
commits into
master
Choose a base branch
from
fix/sdk-7121-tra-support-file-race
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+128
−43
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
b5f9d49
fix(observability): inject TRA support-file listener synchronously (S…
osho-20 47a6f39
test(observability): cover accessibility path + idempotency (SDK-7121)
osho-20 fed11f7
Merge branch 'master' into fix/sdk-7121-tra-support-file-race
osho-20 3de3f02
fix(accessibility): preserve build-start exception log on glob.sync f…
osho-20 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| 'use strict'; | ||
| const chai = require('chai'); | ||
| const expect = chai.expect; | ||
| const sinon = require('sinon'); | ||
| const fs = require('fs'); | ||
| const os = require('os'); | ||
| const path = require('path'); | ||
|
|
||
| const o11yHelper = require('../../../../bin/testObservability/helper/helper'); | ||
| const a11yHelper = require('../../../../bin/accessibility-automation/helper'); | ||
| const baseHelper = require('../../../../bin/helpers/helper'); | ||
|
|
||
| // Regression guard for SDK-7121: the support-file instrumentation MUST land | ||
| // synchronously. runs.js calls setEventListeners(bsConfig) and then proceeds | ||
| // immediately to md5 hashing + zip archiving. When the injection was deferred to | ||
| // an async glob callback, it raced the archive — a lost race shipped an | ||
| // un-instrumented suite, and md5 caching made it sticky, so the new Automate | ||
| // dashboard (TRA) received zero test events. | ||
| describe('SDK-7121 synchronous support-file instrumentation', () => { | ||
| let tmpDir, supportPath, cwdStub, getSupportFilesStub; | ||
|
|
||
| const setupTmpProject = () => { | ||
| tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'sdk7121-')); | ||
| fs.mkdirSync(path.join(tmpDir, 'cypress', 'support'), { recursive: true }); | ||
| supportPath = path.join(tmpDir, 'cypress', 'support', 'e2e.js'); | ||
| fs.writeFileSync(supportPath, '// user original support file\n'); | ||
| cwdStub = sinon.stub(process, 'cwd').returns(tmpDir); | ||
| }; | ||
|
|
||
| afterEach(() => { | ||
| if (cwdStub) cwdStub.restore(); | ||
| if (getSupportFilesStub) getSupportFilesStub.restore(); | ||
| if (tmpDir) fs.rmSync(tmpDir, { recursive: true, force: true }); | ||
| cwdStub = getSupportFilesStub = tmpDir = undefined; | ||
| }); | ||
|
|
||
| describe('testObservability setEventListeners', () => { | ||
| beforeEach(() => { | ||
| setupTmpProject(); | ||
| process.env.BS_TESTOPS_BUILD_COMPLETED = 'true'; | ||
| // non-magic path -> glob.sync resolves the exact file | ||
| getSupportFilesStub = sinon.stub(baseHelper, 'getSupportFiles').returns({ | ||
| supportFile: '/cypress/support/e2e.js', | ||
| cleanupParams: {} | ||
| }); | ||
| }); | ||
|
|
||
| it('injects the observability require synchronously before returning', () => { | ||
| o11yHelper.setEventListeners({ run_settings: {} }); | ||
| // Read exactly as md5/archive would — synchronously, right after the call. | ||
| const content = fs.readFileSync(supportPath, 'utf-8'); | ||
| expect(content).to.include('browserstack-cypress-cli/bin/testObservability/cypress'); | ||
| }); | ||
|
|
||
| it('does not double-inject when called twice (idempotent)', () => { | ||
| o11yHelper.setEventListeners({ run_settings: {} }); | ||
| o11yHelper.setEventListeners({ run_settings: {} }); | ||
| const content = fs.readFileSync(supportPath, 'utf-8'); | ||
| const occurrences = content.split('browserstack-cypress-cli/bin/testObservability/cypress').length - 1; | ||
| expect(occurrences).to.equal(1); | ||
| }); | ||
| }); | ||
|
|
||
| describe('accessibility setAccessibilityEventListeners (glob-pattern branch)', () => { | ||
| beforeEach(() => { | ||
| setupTmpProject(); | ||
| // magic pattern -> exercises the glob.sync branch fixed for SDK-7121 | ||
| getSupportFilesStub = sinon.stub(baseHelper, 'getSupportFiles').returns({ | ||
| supportFile: '/cypress/support/**/*.js', | ||
| cleanupParams: {} | ||
| }); | ||
| }); | ||
|
|
||
| it('injects the accessibility require synchronously before returning', () => { | ||
| a11yHelper.setAccessibilityEventListeners({ run_settings: {} }); | ||
| const content = fs.readFileSync(supportPath, 'utf-8'); | ||
| expect(content).to.include('browserstack-cypress-cli/bin/accessibility-automation/cypress'); | ||
| }); | ||
| }); | ||
| }); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are removing this, how do we know if exception caused issue in build start?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exception visibility is preserved — actually improved. With
glob.sync, any glob failure now throws instead of being handed to a callbackerrarg, and that throw is caught by the outertry…catch(e)wrapping the whole function:So a build-start parse failure is still logged — and with more signal than before: the removed callback only logged a static string (
EXCEPTION IN BUILD START EVENT : Unable to parse cypress support files) with no error object, whereas the outer catch logs the actual${e}and forwardstrue, eto the reporter.This also brings it in line with
testObservability.setEventListeners, which is the pattern this PR mirrors — it uses the sameglob.sync+ single outer catch and never had a separate glob-error branch either.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in code (commit 3de3f02): wrapped
glob.syncin its owntry/catchthat restores the distinctiveEXCEPTION IN BUILD START EVENT : Unable to parse cypress support fileslog on a parse failure (now with the error object attached) and returns early — keeping the call synchronous to avoid the archive race.