@W-24278591 fix: Complete AuthFlowTester nightly coverage - #3050
Conversation
JohnsonEricAtSalesforce
left a comment
There was a problem hiding this comment.
Verdict
Approve on the merits — 110-count, group-disjointness, and retry-merge/validation logic all check out on direct read-through and hand-trace. One requested change (silent-failure risk) worth addressing before merge; the rest is non-blocking.
Separately: ui-tests-pr's green result on this PR isn't real validation of its own changes. pr.yaml triggers on pull_request_target and calls the reusable workflow via an unpinned local uses: ./... path, so GitHub resolves the workflow file from dev, not this branch — confirmed by the executed job's step names (Run All Single User Tests, Run All Multi User Tests, Validate Complete PR Test Results), which only exist in dev's current copy, not this PR's diff. This is a pre-existing, repo-wide CI-resolution gap, not something this PR introduced — flagging in case it's not already known, not asking this PR to fix it. Real validation of these workflow changes only happens post-merge.
Requested Changes (ranked, actionable)
-
parse_bounded()'sValueErrorfails silently.reusable-ui-workflow.yaml, "Copy Test Results" step:continue-on-error: truewith no catch/log around the retry-merge Python means a tripped XML guard (oversized file, or<!DOCTYPE/<!ENTITY) drops that one(group, API level)'s retry merge with no annotation, no job-summary trace, and "Validate Complete Test Results" doesn't gate on this step's outcome — a downstream count mismatch would give no signal thatparse_boundedwas the cause. Suggest anecho "::warning::..."before the guard lets the step continue. -
DEVICE_ARGSfor-loop duplicated verbatim across 4 steps ("Run Login Tests", "Run Welcome Discovery Tests", "Run Token Lifecycle Tests", "Run All Multi User Tests") — differs only in--test-targets/--timeout/GCLOUD_RESULTS_DIR. Worth factoring into one shared step/function so future device-range edits don't need 4 synchronized copies.
Open Questions (ranked, non-blocking)
- Does the
duplicate_keyscross-group check catch a coincidental double-fault (one test miscounted into the wrong group while another drops out elsewhere, netting back to 110)? As written it only catches actual key repeats, not an offsetting stray/missing pair. Low real-world likelihood — confirming whether that's an accepted limitation.
This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce.
|
@JohnsonEricAtSalesforce Thanks for the detailed review. Addressed the requested silent-failure issue in 325a516: retry-merge failures now emit an explicit GitHub warning with the group and API level, preserve the original Firebase result, and still clean up the temporary rerun file. The XML size and DTD/entity guards remain unchanged. On the open question: yes, an offsetting unique stray/missing pair could theoretically keep both the per-group and aggregate counts unchanged. The current checks catch missing groups, per-group runner/result count mismatches, cross-group duplicates, and aggregate drift, but they do not prove exact set equality against a canonical test manifest. That is an accepted limitation for this change; closing it would require deriving or maintaining the expected logical test identities rather than only the expected count of 110. I left the repeated Validation: workflow YAML parses, |
JohnsonEricAtSalesforce
left a comment
There was a problem hiding this comment.
Verified 325a516c0 — the if ! python3 ...; then ... fi wrap attaches the heredoc correctly, leaves the original result file untouched when the guard trips, and still runs cleanup unconditionally. The warning now names the group and API level.
Approving.
This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce.
Summary
Root cause
The previous single-user invocation reached Firebase Test Lab's 60-minute limit. Late-running suites were absent from the artifacts, while retry records were appended to the original JUnit cases and could inflate totals and failures.
Validation
The Firebase matrix needs organization secrets and the forcedotcom runner. In addition, the current
pull_request_targetcaller resolves local reusable workflows from the base branch, so the green PR UI-test check does not exercise this branch's reusable-workflow changes. The new completeness gate will receive its end-to-end validation after merge; the broader PR-validation gap should be handled separately.Companion Workspace PR: https://git.soma.salesforce.com/SalesforceMobileSDK/SalesforceMobileSDK-Workspace/pull/115
Resolves W-24278591