Skip to content

feat(compiler): Add fixedOrder option to expectDiagnostics #7453

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

harlanenciso112
Copy link

@harlanenciso112 harlanenciso112 commented May 23, 2025

Resolves #5818

Description

Added a new fixedOrder option to the expectDiagnostics function that allows testing diagnostics regardless of the order they appear in. This makes tests more resilient to code changes that might alter diagnostic order without changing the actual diagnostic content.

Problem

Tests that use expectDiagnostics would fail when diagnostic order changes, even though this should not be considered a breaking change. Library test authors had to create wrappers to pre-sort diagnostics collections.

Solution

  • Added fixedOrder option to expectDiagnostics (defaults to true for backward compatibility)
  • When fixedOrder: false, diagnostics can match in any order
  • Maintains existing behavior when using the default options

Testing

All existing tests pass, confirming that backward compatibility is maintained.

Changes

  • Minor: Added new option to existing function

@harlanenciso112
Copy link
Author

@microsoft-github-policy-service agree

Copy link
Member

Choose a reason for hiding this comment

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

we use something else than changeset for this repo called chronus. Either follow the comment that should get added soon or use pnpm chronus add

Copy link
Contributor

❌ There is undocummented changes. Run chronus add to add a changeset or click here.

The following packages have changes but are not documented.

  • @typespec/compiler
Show changes

expectation.code,
`Diagnostic at index ${i} has non matching code.\n${message}`,

if (fixedOrder) {
Copy link
Member

Choose a reason for hiding this comment

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

The approach we decided here was to sort the diagnostics so they diff can still be used.
The reason I didn't want to add this at it was was exactly because it made the test experience worse when it didn't find the match and in general it was bad pattern to have multiple diagnostics tested at the same time. We decided that sorting the diagnositcs and expectation would still provide the benefits while not loosing the diff

Copy link
Member

@timotheeguerin timotheeguerin left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, I think unfortunately we'd prefer to go with a bit of a different approach for this.

Have you found yourself needing this feature?

@harlanenciso112
Copy link
Author

Thanks for your feedback @timotheeguerin!

I understand your preference for sorting the diagnostics instead of the fixedOrder approach. That makes sense - preserving the diff functionality would provide better feedback when tests fail.

I implemented this feature based on the issue description which suggested adding a fixedOrder option. I haven't personally needed this feature, but was working on resolving the open issue.

Regarding the changeset, I tried to create one manually but wasn't familiar with Chronus. I'll try using pnpm chronus add as you suggested.

Would you like me to:

  1. Close this PR and create a new one with the sorting approach instead?
  2. Modify this PR to implement sorting of diagnostics?

Thanks for the guidance!

@timotheeguerin
Copy link
Member

You can keep modifying this PR, and yeah sorry about the issue description we have not been doing a good job of relecting our internal discussions on them

For chronus you can also just use that comment which has a link to add it #7453 (comment)

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.

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