Skip to content
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

Make AnalyzerResults deterministically ordered #198

Merged
merged 2 commits into from
Feb 14, 2022

Conversation

0xced
Copy link
Contributor

@0xced 0xced commented Feb 13, 2022

The order itself is not relevant (the NuGetFrameworkSorter used to sort target frameworks doesn't guarantee any specific order anyway) but the order is deterministic, meaning that several builds of the same project will always yield the same order of the target frameworks and results.

The nondeterministic nature of the current implementation made it hard to diagnose an implementation issue in Stryker.NET: stryker-mutator/stryker-net#1899 (comment)

@daveaglick
Copy link
Collaborator

Thanks! Looks good at first glance. Trying to get a strange test failure to clear in CI (it passes locally in Windows but not on the Windows agent, and passes the other OS agents so I'm a little perplexed) and then I'll take a closer look at this PR.

@0xced
Copy link
Contributor Author

0xced commented Feb 13, 2022

I see that the test causing issues uses Build(options).First() which is non-deterministic! This is exactly what this pull request is about, so you should also try to run the tests on this PR. I think that might solve this issue. 😉

Skip only those that don't actually pass (using NUnit's [Platform("win")] attribute)
The order itself is not relevant (the `NuGetFrameworkSorter` used to sort target frameworks doesn't guarantee any specific order anyway) but the order is deterministic, meaning that several builds of the same project will always yield the same order of the target frameworks and results.

The nondeterministic nature of the current implementation made it hard to diagnose an implementation issue in Stryker.NET: stryker-mutator/stryker-net#1899 (comment)
@daveaglick
Copy link
Collaborator

FYI - I did a rebase and force push to trigger the CI build/test reporting again. Still working on getting that just right, and seemed like the only way to restart from the refreshed GitHub Actions workflows. Assuming all the tests pass I'll get this merged.

@0xced
Copy link
Contributor Author

0xced commented Feb 14, 2022

✅ It passed! 🥳

@daveaglick
Copy link
Collaborator

:shipit:

I'll get the next Buildalyzer release out momentarily.

@daveaglick daveaglick merged commit 2ae1749 into phmonte:main Feb 14, 2022
@0xced 0xced deleted the OrderedAnalyzerResults branch February 14, 2022 22:36
0xced added a commit to 0xced/stryker-net that referenced this pull request Dec 30, 2023
Just like stryker-mutator#1900 but for the InputFileResolver.

This was not an issue until phmonte/Buildalyzer#198 was reverted in phmonte/Buildalyzer@8e85a15 for the Buildalyzer 6.0.1 release. Before this change in Buildalyzer, the empty target framework was sorted last but since v6.0.1 it's sorted first instead.

Also, improve the NotSupportedException message when the language is undefined because `System.NotSupportedException: Specified method is not supported` is a terrible message.
rouke-broersma pushed a commit to stryker-mutator/stryker-net that referenced this pull request Dec 30, 2023
Make sure to select valid IAnalyzerResult from Buildalyzer

Just like #1900 but for the InputFileResolver.

This was not an issue until phmonte/Buildalyzer#198 was reverted in phmonte/Buildalyzer@8e85a15 for the Buildalyzer 6.0.1 release. Before this change in Buildalyzer, the empty target framework was sorted last but since v6.0.1 it's sorted first instead.

Also, improve the NotSupportedException message when the language is undefined because `System.NotSupportedException: Specified method is not supported` is a terrible message.
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.

2 participants