Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Jun 7, 2025

The --timeout option parsing was using the current culture for float.TryParse(), which could cause parsing failures in locales that use different decimal separators (e.g., comma instead of period in German locale).

Changes Made

Fixed two locations where timeout values are parsed:

  1. PlatformCommandLineProvider.cs - Validation logic now uses CultureInfo.InvariantCulture
  2. TestHostBuilder.cs - Parsing logic now uses CultureInfo.InvariantCulture

Before

// This would fail in German locale for "1.5s"
float.TryParse(arg[..(size - 1)], out float value)

After

// This works consistently across all locales
float.TryParse(arg[..(size - 1)], CultureInfo.InvariantCulture, out float value)

Tests Added

Added comprehensive unit tests to verify:

  • Valid timeout formats work across different cultures (en-US, de-DE, fr-FR)
  • Invalid formats including culture-specific decimal separators are rejected
  • Parsing specifically uses invariant culture vs current culture

The fix ensures that timeout values like --timeout 1.5s work consistently regardless of the user's system locale.

Fixes #5451.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI changed the title [WIP] MTP's --timeout parsing uses current culture, it should use invariant Fix MTP timeout parsing to use invariant culture instead of current culture Jun 7, 2025
Copilot AI requested a review from Youssef1313 June 7, 2025 16:27
Copy link
Contributor Author

Copilot AI commented Jun 8, 2025

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • 4vyvsblobprodcus361.vsblob.vsassets.io
    • Triggering command: /home/REDACTED/work/testfx/testfx/.dotnet/dotnet msbuild /m /nologo /clp:Summary /v:minimal /nr:true /warnaserror /p:TreatWarningsAsErrors=true /p:ContinuousIntegrationBuild=false /home/REDACTED/work/testfx/testfx/artifacts/toolset/restore.proj /t:__WriteToolsetLocation /clp:ErrorsOnly;NoSummary /p:__ToolsetLocationOutputFile=/home/REDACTED/work/testfx/testfx/artifacts/toolset/10.0.0-beta.25305.3.txt (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copy link
Contributor Author

Copilot AI commented Jun 8, 2025

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • 4vyvsblobprodcus361.vsblob.vsassets.io
    • Triggering command: dotnet build src/Platform/Microsoft.Testing.Platform/Microsoft.Testing.Platform.csproj --no-restore (dns block)
  • 8xbvsblobprodcus382.vsblob.vsassets.io
    • Triggering command: dotnet build src/Platform/Microsoft.Testing.Platform/Microsoft.Testing.Platform.csproj --no-restore (dns block)
  • bcnvsblobprodcus378.vsblob.vsassets.io
    • Triggering command: dotnet build src/Platform/Microsoft.Testing.Platform/Microsoft.Testing.Platform.csproj --no-restore (dns block)
  • i1qvsblobprodcus353.vsblob.vsassets.io
    • Triggering command: dotnet build src/Platform/Microsoft.Testing.Platform/Microsoft.Testing.Platform.csproj --no-restore (dns block)
  • l49vsblobprodcus358.vsblob.vsassets.io
    • Triggering command: dotnet build src/Platform/Microsoft.Testing.Platform/Microsoft.Testing.Platform.csproj --no-restore (dns block)
  • mfjvsblobprodcus373.vsblob.vsassets.io
    • Triggering command: dotnet build src/Platform/Microsoft.Testing.Platform/Microsoft.Testing.Platform.csproj --no-restore (dns block)
  • s4uvsblobprodcus326.vsblob.vsassets.io
    • Triggering command: dotnet build src/Platform/Microsoft.Testing.Platform/Microsoft.Testing.Platform.csproj --no-restore (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copy link
Contributor Author

Copilot AI commented Jun 10, 2025

@Youssef1313 Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub.

You can ask me to try again later by mentioning me in a new comment.

If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: 89ce298f-5839-4db3-8b2f-dd5652baac9f

Sorry for the inconvenience!

Copy link
Contributor Author

Copilot AI commented Jun 10, 2025

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • 4vyvsblobprodcus361.vsblob.vsassets.io
    • Triggering command: /home/REDACTED/work/testfx/testfx/.dotnet/dotnet msbuild /m /nologo /clp:Summary /v:minimal /nr:true /warnaserror /p:TreatWarningsAsErrors=true /p:ContinuousIntegrationBuild=false /home/REDACTED/work/testfx/testfx/artifacts/toolset/restore.proj /t:__WriteToolsetLocation /clp:ErrorsOnly;NoSummary /p:__ToolsetLocationOutputFile=/home/REDACTED/work/testfx/testfx/artifacts/toolset/10.0.0-beta.25305.3.txt (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copy link
Contributor Author

Copilot AI commented Jun 10, 2025

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • 4vyvsblobprodcus361.vsblob.vsassets.io
    • Triggering command: /home/REDACTED/work/testfx/testfx/.dotnet/dotnet msbuild /m /nologo /clp:Summary /v:minimal /nr:true /warnaserror /p:TreatWarningsAsErrors=true /p:ContinuousIntegrationBuild=false /home/REDACTED/work/testfx/testfx/artifacts/toolset/restore.proj /t:__WriteToolsetLocation /clp:ErrorsOnly;NoSummary /p:__ToolsetLocationOutputFile=/home/REDACTED/work/testfx/testfx/artifacts/toolset/10.0.0-beta.25305.3.txt (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 73.90%. Comparing base (1421741) to head (e69cb8c).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
...icrosoft.Testing.Platform/Hosts/TestHostBuilder.cs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5705      +/-   ##
==========================================
- Coverage   76.37%   73.90%   -2.48%     
==========================================
  Files         602      602              
  Lines       36664    36664              
==========================================
- Hits        28003    27095     -908     
- Misses       8661     9569     +908     
Flag Coverage Δ
Debug 73.90% <50.00%> (-2.48%) ⬇️
integration 73.90% <50.00%> (-2.48%) ⬇️
production 73.90% <50.00%> (-2.48%) ⬇️
unit 73.90% <50.00%> (-2.48%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...latform/CommandLine/PlatformCommandLineProvider.cs 88.49% <100.00%> (-3.54%) ⬇️
...icrosoft.Testing.Platform/Hosts/TestHostBuilder.cs 91.42% <0.00%> (-3.35%) ⬇️

... and 59 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Youssef1313 Youssef1313 marked this pull request as ready for review June 10, 2025 12:43
@Youssef1313 Youssef1313 merged commit 9f028f3 into main Jun 10, 2025
10 checks passed
@Youssef1313 Youssef1313 deleted the copilot/fix-5451 branch June 10, 2025 15:10
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.

MTP's --timeout parsing uses current culture, it should use invariant

4 participants