[WIP] Add logging for non-bug-related test failures in automated tests. - #6084
michelinewu wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Improves automated test triage by classifying common non-bug failure modes (shutdown issues, account/user-pool issues, and analytics-post failures) and persisting structured “account failure” metadata for both per-test and job-wide reporting.
Changes:
- Adds more specific shutdown-failure logging by normalizing unknown thrown values into readable messages.
- Detects account-related failure reasons from user-pool exhaustion and app logs, and records them to
test-dist/account-failures.json. - Extends the CI runner analytics payload with per-test and job-level
accountFailurefields, and records a job-wide reason if analytics submission fails.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/helpers/webdriver/user.ts | Tracks user-pool exhaustion state for the current test to support downstream failure classification. |
| test/helpers/webdriver/runner-utils.ts | Defines account-failure types and writes per-test account failure reasons to a shared JSON file. |
| test/helpers/webdriver/index.ts | Captures per-test log slices for failure detection and improves shutdown error formatting/logging. |
| test/helpers/runner.js | Aggregates and reports account-failure reasons in analytics; records job-wide reasons when analytics submission fails. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| function readAccountFailures(): IAccountFailures { | ||
| const failures = fs.existsSync(ACCOUNT_FAILURES_PATH) | ||
| ? JSON.parse(fs.readFileSync(ACCOUNT_FAILURES_PATH, 'utf8')) | ||
| : {}; | ||
| return { tests: failures.tests ?? {}, job: failures.job ?? [] }; | ||
| } |
| * This is important because it indicates that the test failure is not a bug in the app. | ||
| * @returns - The type for the account reason the test failed | ||
| */ | ||
| function detectAccountFailure(): TAccountFailure { |
BundleMonUnchanged files (4)
No change in files bundle size Final result: ✅ View report in BundleMon website ➡️ |
| */ | ||
| export function saveAccountFailureToFile(testName: string, reason: TAccountFailure) { | ||
| const failures = readAccountFailures(); | ||
| failures.tests[testName] = reason; |
There was a problem hiding this comment.
Storing account failures by test name makes the reason survive into the retry run. If the first attempt fails with a YouTube/account issue and the retry then fails for a real app bug, sendJobToAnalytics() will still attach the stale accountFailure to that unrecovered failure. We should clear this test's account-failure entry before retrying/rerunning it, or record attempts separately, so final failures are not mislabeled as account noise.
| await requestUtilityServer('job', 'post', body); | ||
| } catch (e) { | ||
| console.error('failed to send analytics', e); | ||
| saveJobAccountFailure('HEROKU_ANALYTICS_SEND_FAILED'); |
There was a problem hiding this comment.
This records HEROKU_ANALYTICS_SEND_FAILED only after the analytics POST has already failed. The runner reads account-failures.json before this request, and the Azure job does not publish test-dist, so this reason never reaches analytics or any artifact; it only lands in a local file that disappears with the job. Either include this condition in something observable before/when the POST fails, or drop the file write and rely on the existing console error.
michelinewu
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
test/helpers/webdriver/runner-utils.ts:56
- PR description says results are written to
test-dist/account-failures.json, but the implementation writes totest-dist/failure-reasons.json. This mismatch makes it harder to locate artifacts and reason about the change. Please align the code and PR description (including any references in the runner/harness) to a single filename/shape.
const FAILURE_REASONS_PATH = 'test-dist/failure-reasons.json'; // failures because of known frequent non-bug reasons (like account issues) should be written in this file
test/helpers/runner.js:115
readFailureReason()can throw iffailure-reasons.jsonparses to a non-object value (e.g.null), becausefailures.tests/failures.jobare accessed outside the try/catch. This would crash the runner while sending analytics.
} catch (e) {
failures = {};
}
return { tests: failures.tests || {}, job: failures.job || [] };
}
| const failureReason = detectFailureReason(); | ||
| if (detectAccountFailure(failureReason)) { | ||
| console.log(`Test failed because of failure reason: ${failureReason}`); | ||
| } | ||
| saveFailureReasonToFile(testName, failureReason); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (4)
test/helpers/webdriver/index.ts:570
detectFailureReason()can returnnull, but its result is always passed intosaveFailureReasonToFile. This triggers the "Expected failure reason but none provided" log for normal app-bug failures and prevents the failure-reasons file from being written, which in turn makes the runner log an error when it tries to read the file.
const failureReason = detectFailureReason();
if (detectAccountFailure(failureReason)) {
console.log(`Test failed because of failure reason: ${failureReason}`);
}
saveFailureReasonToFile(testName, failureReason);
test/helpers/webdriver/index.ts:434
logsFromCurrentTestis intended to contain only logs written since the current test began, butlogFileLastReadingPosis later set tologs.length - 1, so appendinglogs.slice(logFileLastReadingPos)re-includes the last character from the previous read. This can cause cross-test log contamination and (rarely) false-positive regex matches.
const logs: string = await readLogs();
if (!logs) return;
lastLogs = logs;
logsFromCurrentTest += logs.slice(logFileLastReadingPos);
let ignoringErrors = false;
test/helpers/runner.js:115
readFailureReason()always attempts toreadFileSyncthe failure-reasons file. When the file doesn't exist (e.g., no failures, or failures with no recognized reason), this throws ENOENT and logs an error that looks like a harness failure. Consider treating a missing file as the normal empty case and only logging when the file exists but can't be parsed.
function readFailureReason() {
try {
const failures = JSON.parse(fs.readFileSync(failureReasonsFile, 'utf8'));
return { tests: failures?.tests || {}, job: failures?.job || [] };
} catch (e) {
test/helpers/webdriver/runner-utils.ts:39
- The PR description says results are written to
test-dist/account-failures.jsonand includes a job-level reason likeHEROKU_ANALYTICS_SEND_FAILED. The implementation writestest-dist/failure-reasons.jsonand theTFailureReasonunion doesn't include the described job-level reason (and the runner currently doesn't write anyjobreasons). Either update the PR description to match the implemented format, or adjust the code + runner to write/read the documented file name and reason set.
export type TFailureReason =
| 'MISSING_TRANSLATIONS'
| 'UNMOUNTED_REACT_UPDATE'
| 'NO_ACCOUNT_AVAILABLE'
| 'YOUTUBE_STREAMING_DISABLED'
| 'YOUTUBE_ACCOUNT_RATE_LIMITED'
| 'YOUTUBE_ACCOUNT_FAILURE'
| 'UTILITY_SERVER_FAILURE';
Specify details for automated test failure cases
Automated test failure logs were too generic to rule out non-bug related failures.
Issues
**Every shutdown failure reported the same message.**Errors arrive as
unknowncausing webdriver to reject withErrorobjects when sometimes the api-client rejects with plain strings and with serialized error objects. ThecatchinstopAppemitted a blanket'Crash on shutdown'regardless of what was thrown. That one message covers at least four unrelated conditions:waitForElectronInstancesExisttimed out.closeWindowreached it.Account-caused failures looked identical to app bugs.
Failures due to issues from the user pool accounts, rather than app-related bugs, were logged the same in summary, requiring triage by hand in the logs for the specific test failure. This is unnecessarily time consuming. Specific cases:
3. The analytics POST failing was silent.
sendJobToAnalyticswraps its request in acatchthat logs and swallows it, so the entire body, including failed tests and stats, was discarded with no trace beyond one console line. This meant that test failures due to failure to post analytics was logged asECONNABORTEDand not distinguished from test failures due to app bugs.Fixes
Shutdown errors are classified An error is passed as
unknownand is now identified so it can be logged in a readable way. Note: the fallback case usesutil.inspectrather thanString()orJSON.stringify.String()renders any object without a.messageas[object Object], so the message will not actually be logged.JSON.stringifyis unsafe because it returns the valueundefinedforundefined/symbols, and it throws outright on circular references.util.inspecthandles both and matches howmain.jsandapp/app.tsalready serialize unknown values into the log.Account failures are detected and recorded. A new
TAccountFailureunion names the reasons a run failed because of its accounts:NO_ACCOUNT_AVAILABLEreserveUserFromPoolwhere it gives up after 5 attemptsYOUTUBE_STREAMING_DISABLEDliveStreamingNotEnabledin the app logYOUTUBE_ACCOUNT_RATE_LIMITEDuserRequestsExceedRateLimitin the app logYOUTUBE_ACCOUNT_FAILUREHEROKU_ANALYTICS_SEND_FAILEDThe user pool case
NO_ACCOUNT_AVAILABLEis flagged at its source rather than string-matched because the harness never sees the AVA error. The YouTube cases come from the log because that is the only place the reason surfaces.Detection reads
currentTestLogs, which accumulates only the slice written since the current test began because the pre-existinglastLogsholds the whole file and leaks across tests whenever the cache directory is reused.Results are written to
test-dist/account-failures.jsonFor extensibility, write results under a shape shared by both writers, described by the exportedIAccountFailures:{ "tests": { "Go live to YouTube": "YOUTUBE_ACCOUNT_RATE_LIMITED" }, "job": ["HEROKU_ANALYTICS_SEND_FAILED"] }testsis keyed by test name and written by the harness;jobholds run-wide reasons and is written by the runner. Both sides read through a normalizer that fills in the missing halves, so an absent, empty, or older flat-shaped file degrades to empty instead of throwing. Job reasons are appended rather than assigned, so an earlier reason is not lost behind a later one.Analytics carries the reason. Each entry in
failedTestsgains anaccountFailurefield, and the body gains a job-levelaccountFailurechosen off an explicitACCOUNT_FAILURE_SEVERITYordering rather than first-seen.File(s) changed:
test/helpers/webdriver/index.ts,test/helpers/webdriver/runner-utils.ts,test/helpers/webdriver/user.ts,test/helpers/runner.jsPerformance Implications
None. All work happens in
afterEach.alwayson the failure path only, after the test has already failed. Detection is a few regex matches over a string already held in memory, and the file write is a few hundred bytes totest-dist/. No change to the passing path.