Skip to content

@W-24278591 fix: Complete AuthFlowTester nightly coverage - #3050

Merged
wmathurin merged 2 commits into
forcedotcom:devfrom
wmathurin:codex/authflow-nightly-completeness
Sep 23, 2026
Merged

wmathurin merged 2 commits into
forcedotcom:devfrom
wmathurin:codex/authflow-nightly-completeness

Conversation

@wmathurin

@wmathurin wmathurin commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • split AuthFlowTester nightlies into four sequential, disjoint groups across Android API 31–37
  • replace an original Firebase result with its final retry outcome instead of counting both attempts
  • require all four groups and exactly 110 unique logical test executions per API level
  • surface guarded retry-merge failures as GitHub warnings while preserving the original result
  • update the AuthFlowTester README with the current inventory and grouping

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

  • actionlint: no new workflow errors; existing custom-runner and shell-style warnings remain
  • workflow YAML syntax validation passes
  • source inventory: 104 declared tests plus 6 inherited AdvancedAuthBeacon executions = 110 per API
  • git diff --check

The Firebase matrix needs organization secrets and the forcedotcom runner. In addition, the current pull_request_target caller 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

@JohnsonEricAtSalesforce JohnsonEricAtSalesforce left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

  1. parse_bounded()'s ValueError fails silently. reusable-ui-workflow.yaml, "Copy Test Results" step: continue-on-error: true with 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 that parse_bounded was the cause. Suggest an echo "::warning::..." before the guard lets the step continue.

  2. DEVICE_ARGS for-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)

  1. Does the duplicate_keys cross-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.

@wmathurin

Copy link
Copy Markdown
Contributor Author

@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 DEVICE_ARGS construction unchanged because it is non-blocking and factoring state across four independent Actions shell steps would broaden this workflow change. I also verified the pull_request_target observation from the executed job's old step names and documented the pre-existing PR-validation gap in the PR description for separate follow-up.

Validation: workflow YAML parses, git diff --check passes, and actionlint reports no new findings (only the existing custom-runner and shell-style warnings).

@wmathurin
wmathurin merged commit bd8f6d2 into forcedotcom:dev Sep 23, 2026
10 checks passed

@JohnsonEricAtSalesforce JohnsonEricAtSalesforce left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants