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

Unable to instrument module because : Input string was not in a correct format #998

Closed
heaths opened this issue Nov 23, 2020 · 3 comments · Fixed by #1000
Closed

Unable to instrument module because : Input string was not in a correct format #998

heaths opened this issue Nov 23, 2020 · 3 comments · Fixed by #1000
Labels
bug Something isn't working

Comments

@heaths
Copy link
Contributor

heaths commented Nov 23, 2020

Using VSTest integration, we generate coverage reports for our primarily netstandard2.0-targeting assemblies. However, for a subset of assemblies (seemingly the first in a directory listing per service directory, e.g. in https://github.com/azure/azure-sdk-for-net: Azure.Core under sdk/core and Azure.Security.KeyVault.Administration in sdk/keyvault) diagnostic lots on the CI (Azure Pipelines) show something like:

TpTrace Warning: 0 : 1076, 1, 2020/11/21, 02:02:34.280, 5338411430, datacollector.dll, [coverlet]Unable to instrument module: 'D:\a\1\s\artifacts\bin\Azure.Security.KeyVault.Administration.Tests\Debug\netcoreapp2.1\Azure.Security.KeyVault.Administration.dll' because : Input string was not in a correct format.

This is from build https://dev.azure.com/azure-sdk/public/_build/results?buildId=629023&view=logs&j=1e00682f-6081-5348-8da2-7d601664b2e3&t=4f3e1a81-cacf-5ad2-6e75-5049507bfdad (should be retained for ~30 days).

Running the same commands (just different source path) locally yields the expected results, e.g. Azure.Core is covered, as is Azure.Security.KeyVault.Administration.

Looking at other issues here, it seems the FormatException that's being thrown (based on the error message) is most often from targeting < netstandard2.0, but our baseline already is netstandard2.0. Our common root dependency, Azure.Core, targets net461 and netstandard2.0 (for those few rare classes in net461 that aren't actually in netstandard2.0) but, either way, should work according to other issue conversations herein.

We are using the latest GA for Microsoft.NET.Test.Sdk, 16.8.0.

Update: setting /p:ContinuousIntegrationBuild=true on the local repro (same command line as CI otherwise; that property is set via targets in CI based on $TF_BUILD) repro'd locally. I'll attempt to debug into the problem.

@heaths
Copy link
Contributor Author

heaths commented Nov 23, 2020

After debugging into this, I found the problem. We have a few classes that put generic params in the file name with curly braces like so (it's a shared file several projects inport):

Mapping resolved: '/_/sdk/core/Azure.Core.TestFramework/src/RecordedTestBase{TEnvironment}.cs' -> 'C:\src\azure-sdk-for-net\sdk\core\Azure.Core.TestFramework\src\RecordedTestBase{TEnvironment}.cs'

This causes a problem with the code in coverlet.collector.dll!Coverlet.Collector.Utilities.TestPlatformEqtTrace.Verbose(string format, object[] args):

        public void Verbose(string format, params object[] args)
        {
            EqtTrace.Verbose($"[coverlet]{format}", args);
        }

We can rename our files to workaround this, but it might be good to scrub the format parameter. Many years ago in a previous product I worked on, we had a similar problem with GUIDs coming in (Windows Installer ProductCodes when a display name wasn't configured) and simply did a replacement of "{" -> "{{" and "}" -> "}}".

@heaths
Copy link
Contributor Author

heaths commented Nov 23, 2020

Of note, SA1649 when enabled expects file names for generic classes to either elide the type param(s) or use {T}. The https://github.com/dotnet/runtime repo disabled SA1649 (we'd rather not, so instead I disabled those warnings for affected files) and more commonly uses OfT.

@MarcoRossignoli
Copy link
Collaborator

Hi @heaths thanks a lot for the investigation. We should fix with a replace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants