Skip to content

test: retry per-credential integration matrix (do not merge) - #728

Draft
amrit-agarwal-1 wants to merge 11 commits into
mainfrom
test/minter-matrix-retry
Draft

test: retry per-credential integration matrix (do not merge)#728
amrit-agarwal-1 wants to merge 11 commits into
mainfrom
test/minter-matrix-retry

Conversation

@amrit-agarwal-1

Copy link
Copy Markdown
Contributor

Scratch PR to re-run the pat/user split gates and see whether the extra failures reproduce or were tenant weather. Based on the Minter PR branch.

Do not merge.

amrit-agarwal-1 and others added 6 commits September 9, 2026 18:50
… under both

Squashed from 25 commits (kept on backup/minter-pre-rebase).

- Mint a user access token with Minter in CI (ACR scoped token, masked, written
  into tests/.env.integration) so suites whose APIs reject PATs can run at all:
  insightsrtm_ (Agents, Memory, Agent Traces, Governance) and the notification
  service, all previously describe.skip.
- describeIntegration(name, requirement, modes, body, options?) declares a suite
  once and expands it over init modes x credentials, naming cells
  [initMode][authMode]. 'any' runs under every configured credential — PAT and
  user token both, per the decision that neither subsumes the other.
- User-token cells resolve MINTER_BASE_URL when set; PAT cells keep
  UIPATH_BASE_URL. getActiveAuth() gives the raw-fetch call sites the credential
  and host of the cell actually running.
- Fixture ownership: suites create and own what they read rather than depending
  on tenant state; rolling time windows replace hardcoded dates; the traces
  suite falls back to expired spans when the pinned trace ages out.
- Five tests skipped with the blocker named: the notification $filter server
  stall, topic groups absent from the tenant, and three agent-activity data gaps.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…mments

'any' read as 'either one will do' when it means 'run under every configured
credential' — the suite executes once per credential, not once. 'both' says
that. Type, call sites, and docs renamed together; no behaviour change.

Also trimmed the comment blocks that had grown past their value, including a
JSDoc citing a specific CI run number that would not age well.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…drop unused helpers

- Unskip the 18 insightsrtm_ describe blocks in maestro processes/cases/case-instances
  and gate them on the user-token cell instead; they were skipped only because PAT
  is rejected there.
- Remove the unused canAuthenticate() helper.
- Make initializeServices' authMode required — every caller passes it.
- Delete tests/.env.integration after the run so the minted token does not outlive
  the job on a reused runner.
The env var the tests read is now UIPATH_TENANT_ID. The GitHub secret keeps the
UIPATH_TENANT_ID_DEV || UIPATH_TENANT_ID override pair, matching every other
secret in the workflow.
15 of the 18 unskipped insightsrtm_ blocks pass under the user token. The three
that do not fail on tenant data, not auth: getInstanceStats' duration fields are
undefined with no completed instances in the window, and getSlaSummary's
slaDueTime is empty.

Also drops the worker-count comment on the integration step, per review.
The pat and user cells ran back-to-back in one job, so the run cost the sum of
both credentials. Each leg now runs on its own runner via INTEGRATION_AUTH_MODE,
which needs SonarCloud out of the test job: it must see both lcov reports and
must run once, so it moves to a job that downloads and merges them.
Comment on lines +15 to +19
const url = `${getActiveAuth().baseUrl}/${config.orgName}/${config.tenantName}/orchestrator_/odata/TaskCatalogs(${id})`;
await fetch(url, {
method: 'DELETE',
headers: {
Authorization: `Bearer ${config.secret}`,
Authorization: `Bearer ${getActiveAuth().token}`,

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.

getActiveAuth() is called twice, invoking resolveToken and resolveBaseUrl separately each time. Destructure once:

Suggested change
const url = `${getActiveAuth().baseUrl}/${config.orgName}/${config.tenantName}/orchestrator_/odata/TaskCatalogs(${id})`;
await fetch(url, {
method: 'DELETE',
headers: {
Authorization: `Bearer ${config.secret}`,
Authorization: `Bearer ${getActiveAuth().token}`,
const { baseUrl, token } = getActiveAuth();
const url = `${baseUrl}/${config.orgName}/${config.tenantName}/orchestrator_/odata/TaskCatalogs(${id})`;
await fetch(url, {
method: 'DELETE',
headers: {
Authorization: `Bearer ${token}`,

The failures are transient 504s and timeouts from Data Fabric, not assertion
failures — a single retry absorbs them.
@claude

claude Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

✅ No issues found. Checked for bugs and CLAUDE.md compliance.

Comment on lines 34 to +38
const config = getTestConfig();
const attachmentId = config.orchestratorAttachmentId!;
const base = `${getActiveAuth().baseUrl}/${config.orgName}/${config.tenantName}/orchestrator_`;
await fetch(`${base}/odata/Attachments(${attachmentId})`, {
method: 'DELETE',
headers: { Authorization: `Bearer ${getActiveAuth().token}` },

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.

getActiveAuth() is called twice — once for .baseUrl (line 35) and once for .token (line 38) — same pattern as the open thread on task-catalogs.integration.test.ts:19. Destructure once:

Suggested change
const config = getTestConfig();
const attachmentId = config.orchestratorAttachmentId!;
const base = `${getActiveAuth().baseUrl}/${config.orgName}/${config.tenantName}/orchestrator_`;
await fetch(`${base}/odata/Attachments(${attachmentId})`, {
method: 'DELETE',
headers: { Authorization: `Bearer ${getActiveAuth().token}` },
const config = getTestConfig();
const { baseUrl, token } = getActiveAuth();
const base = `${baseUrl}/${config.orgName}/${config.tenantName}/orchestrator_`;
await fetch(`${base}/odata/Attachments(${attachmentId})`, {
method: 'DELETE',
headers: { Authorization: `Bearer ${token}` },

Same fix needed in the create/delete afterAll around lines 123–128.

@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Review summary

New finding posted this run:

  • attachments.integration.test.ts:34-38getActiveAuth() called twice in the getById afterAll (once for .baseUrl, once for .token). Same pattern as the open thread on task-catalogs.integration.test.ts:19. Also present in the create/delete afterAll at lines 123-128. Suggest destructuring once: const { baseUrl, token } = getActiveAuth();

Its reads stall past the default 30s on the shared tenant — 18 timeouts in the
last run, spread across the whole file. Same class and remedy as the schema
suite's hooks in #723: budget headroom, not failure tolerance.
// skip: insightsrtm_ endpoints do not support PAT auth — requires OAuth
// skip: the duration fields (minDurationMs onward) come back undefined — the
// tenant has no completed instances in the window to compute them from.
describe.skip('getInstanceStats', () => {

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.

describe.skip is not allowed for missing test data per the convention updated in this PR's own rules.md update: "Do not use describe.skip for missing test data, missing config, or flakiness."

The reason is now data-based ("no completed instances"), not an auth issue. This PR uses it.skip for identical situations elsewhere (e.g. memory.integration.test.ts, governance.integration.test.ts). Collapse the wrapping describe and move the skip inward:

Suggested change
describe.skip('getInstanceStats', () => {
// skip: the duration fields (minDurationMs onward) come back undefined — the
// tenant has no completed instances in the window to compute them from.
it.skip('should retrieve instance stats for a case', async () => {
const { cases } = getServices();
await testGetInstanceStats(cases, 'cases');
});

// skip: insightsrtm_ endpoints do not support PAT auth — requires OAuth
// skip: the duration fields (minDurationMs onward) come back undefined — the
// tenant has no completed instances in the window to compute them from.
describe.skip('getInstanceStats', () => {

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.

Same describe.skip-for-missing-data violation as cases.integration.test.ts:237. Per the convention updated in this PR, use it.skip for a single skipped test blocked on tenant state (no completed instances to compute from):

Suggested change
describe.skip('getInstanceStats', () => {
// skip: the duration fields (minDurationMs onward) come back undefined — the
// tenant has no completed instances in the window to compute them from.
it.skip('should retrieve instance stats for a process', async () => {
const { maestroProcesses } = getServices();
await testGetInstanceStats(maestroProcesses, 'processes');
});

@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Review summary

New findings posted this run:

  • cases.integration.test.ts:237describe.skip used for missing test data. This PR changes the skip reason from PAT-auth (which was the old permitted exception) to missing tenant data, which the updated rules.md explicitly prohibits for describe.skip. Same fix as the rest of the file: collapse to it.skip.
  • processes.integration.test.ts:302 — Identical issue; same fix.

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.

1 participant