Skip to content

Conversation

@Adityanr
Copy link
Contributor

Fixes #6869

Context

cmd.exe full path retrieval from PATH environment variable and comparison in unit test was case sensitive.

Changes Made

Converted cmd.exe full path to lowercase to repair case sensitivity for flaky unit test fact FindOnPathSucceeds() at ToolTask_Tests.cs

Testing

Debugging, unit test run and code compiling.

Notes

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Awesome. This worked for me -- my main computer passed all tests for the first time in a long time!

Just one nitpick in your implementation.

@Adityanr
Copy link
Contributor Author

comments resolved and tested locally. @rainersigwald

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@Adityanr
Copy link
Contributor Author

Hi. I added one small change as well, for linux and macOS unit tests to pass. Can you please approve that as well for CI if possible? @rainersigwald

Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

Shouldly supports case-insensitive comparison so I think this could also be:

cmdPath.ShouldBe(expectedCmdPath, Case.Insensitive);

in the Windows branch (with expectedCmdPath being the one path on Windows). Looks great with ToUpperInvariant() as well, thank you!

@ladipro ladipro added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Oct 21, 2021
@Adityanr
Copy link
Contributor Author

Shouldly supports case-insensitive comparison so I think this could also be:

cmdPath.ShouldBe(expectedCmdPath, Case.Insensitive);

in the Windows branch (with expectedCmdPath being the one path on Windows). Looks great with ToUpperInvariant() as well, thank you!

oh..thats actually much better...i'll watch out for that..thanks!

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

Labels

merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Spurious Windows test failure in FindOnPathSucceeds

3 participants