Skip to content

Add unordered matching to expectDiagnostics - #11710

Open
Vivek JM (vivekjm) wants to merge 2 commits into
microsoft:mainfrom
vivekjm:feat/unordered-diagnostic-expectations
Open

Add unordered matching to expectDiagnostics#11710
Vivek JM (vivekjm) wants to merge 2 commits into
microsoft:mainfrom
vivekjm:feat/unordered-diagnostic-expectations

Conversation

@vivekjm

Copy link
Copy Markdown

Fixes #5818.

expectDiagnostics currently compares each expected diagnostic with the item at the same array index. That makes multi-diagnostic tests fail when compiler changes only affect emission order.

This adds an opt-in fixedOrder: false mode while retaining ordered, strict matching by default. Unordered matching uses one-to-one assignment rather than a greedy scan, so overlapping expectations such as a broad code match and a more specific message match still find a valid pairing. Existing strict: false behavior continues to allow additional diagnostics.

The change includes focused coverage for default ordering, reversed diagnostics, overlapping matches, non-strict matching, and strict count validation, plus the compiler change entry.

I used an AI coding assistant while preparing and validating this contribution.

Validation:

  • pnpm setup:min
  • pnpm --filter @typespec/compiler build
  • pnpm --filter @typespec/compiler test (4,112 passed, 6 skipped)
  • pnpm --filter @typespec/compiler lint
  • pnpm exec prettier packages/compiler/src/testing/expect.ts packages/compiler/test/testing/expect.test.ts .chronus/changes/unordered-diagnostic-expectations-2026-08-18.md --check
  • pnpm gen-compiler-extern-signature
  • pnpm chronus verify
  • git diff --check

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI 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.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds an option to expectDiagnostics so tests can assert expected diagnostics without depending on emission order.

Changes:

  • Introduces fixedOrder (default true) to optionally match diagnostics regardless of order.
  • Adds an unordered matching implementation that supports overlapping expectations.
  • Adds Vitest coverage and a changelog entry for the new option.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
packages/compiler/src/testing/expect.ts Adds ExpectDiagnosticsOptions.fixedOrder and implements unordered matching logic.
packages/compiler/test/testing/expect.test.ts Adds tests for ordered vs unordered matching behavior and strictness interactions.
.chronus/changes/unordered-diagnostic-expectations-2026-08-18.md Documents the new fixedOrder option as a feature change.
Suppressed comments (1)

packages/compiler/src/testing/expect.ts:254

  • strOrRegexMatches introduces a second (boolean-returning) implementation of string/regex matching alongside the existing matchStrOrRegex. This duplication risks the two paths diverging over time (especially around regex edge cases) and makes it harder to reason about consistency between ordered and unordered matching. Consider refactoring so matchStrOrRegex delegates to strOrRegexMatches (using it as the single source of truth for matching semantics), and then only handles assertion/error-message concerns.
function strOrRegexMatches(value: string, expectation: string | RegExp): boolean {
  if (typeof expectation === "string") {
    return value === expectation;
  }

  const lastIndex = expectation.lastIndex;
  const result = expectation.test(value);
  expectation.lastIndex = lastIndex;
  return result;
}

function matchStrOrRegex(value: string, expectation: string | RegExp, assertMessage: string) {
  if (typeof expectation === "string") {
    strictEqual(value, expectation, assertMessage);
  } else {

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/compiler/src/testing/expect.ts Outdated
Comment thread packages/compiler/src/testing/expect.ts
Comment thread packages/compiler/test/testing/expect.test.ts
Copilot AI review requested due to automatic review settings August 19, 2026 08:40

Copilot AI 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.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (3)

packages/compiler/src/testing/expect.ts:268

  • Using fail(assertMessage) here reduces the actionable information in failures (no actual/expected values, and regex expectation formatting may be unclear). Consider including the actual value and the expectation in the thrown message, or re-introducing assert-based comparisons while preserving RegExp.lastIndex (save/restore around the check) so failures remain as informative as before.
function matchStrOrRegex(value: string, expectation: string | RegExp, assertMessage: string) {
  if (!strOrRegexMatches(value, expectation)) {
    fail(assertMessage);
  }
}

packages/compiler/src/testing/expect.ts:87

  • When strict is false, the failure case array.length > diagnostics.length is still enforced (which makes sense: you can’t satisfy more expectations than diagnostics), but the error text "Expected X diagnostics" can be misleading in non-strict mode. Consider adjusting the message in this branch to something like "Expected at least X diagnostics..." or explicitly mention that there are insufficient diagnostics to satisfy the expectations.
  const strict = options.strict ?? true;
  const fixedOrder = options.fixedOrder ?? true;

  if ((strict && array.length !== diagnostics.length) || array.length > diagnostics.length) {
    fail(
      `Expected ${array.length} diagnostics but found ${diagnostics.length}:\n ${formatDiagnostics(
        diagnostics,
      )}`,
    );
  }

packages/compiler/src/testing/expect.ts:205

  • The unordered matching implementation is a non-trivial bipartite-matching-style algorithm. Adding a brief comment describing the approach (augmenting paths), its intent (avoid greedy mismatch with overlapping expectations), and rough complexity would make future maintenance safer (especially if diagnostic counts grow in some tests).
function hasUnorderedMatch(
  diagnostics: readonly Diagnostic[],
  expectations: readonly DiagnosticMatch[],
): boolean {
  const diagnosticMatches = new Array<number>(diagnostics.length).fill(-1);

  function assignExpectation(expectationIndex: number, visited: boolean[]): boolean {
    for (let diagnosticIndex = 0; diagnosticIndex < diagnostics.length; diagnosticIndex++) {
      if (
        visited[diagnosticIndex] ||
        !diagnosticMatchesExpectation(diagnostics[diagnosticIndex], expectations[expectationIndex])
      ) {
        continue;
      }

      visited[diagnosticIndex] = true;
      const previousExpectation = diagnosticMatches[diagnosticIndex];
      if (previousExpectation === -1 || assignExpectation(previousExpectation, visited)) {
        diagnosticMatches[diagnosticIndex] = expectationIndex;
        return true;
      }
    }
    return false;
  }

  return expectations.every((_, index) =>
    assignExpectation(index, new Array<boolean>(diagnostics.length).fill(false)),
  );
}

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

Labels

compiler:core Issues for @typespec/compiler

Projects

None yet

Development

Successfully merging this pull request may close these issues.

testing: expectDiagnostics() should provide an option to ignore ordering

2 participants