-
Notifications
You must be signed in to change notification settings - Fork 382
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
Skip fsharp construct with unknown
source
#1165
Skip fsharp construct with unknown
source
#1165
Conversation
Document document = metadataReader.GetDocument(docHandle); | ||
string docName = _sourceRootTranslator.ResolveFilePath(metadataReader.GetString(document.Name)); | ||
|
||
Guid languageGuid = metadataReader.GetGuid(document.Language); | ||
// We verify all docs and return false if not all are present in local | ||
// We could have false negative if doc is not a source | ||
// Btw check for all possible extension could be weak approach | ||
// We exlude from the check the autogenerated source file(i.e. source generators) | ||
if (!_fileSystem.Exists(docName) && !docName.EndsWith(".g.cs")) | ||
// We exclude from the check the autogenerated source file(i.e. source generators) | ||
// We exclude special F# construct https://github.com/coverlet-coverage/coverlet/issues/1145 | ||
if (!_fileSystem.Exists(docName) && !docName.EndsWith(".g.cs") && !IsUnknownModuleInFSharpAssembly(languageGuid, docName)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic could be extracted into a method, since it is identical with the logic in PortablePdbHasLocalSource
. This helps if we need to add even more exceptions, e.g. for the null documents in VB (#1073).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits but here we go thx a lot!
test/Directory.Build.props
Outdated
<Choose> | ||
<When Condition="$(MSBuildProjectName)!='coverlet.tests.projectsample.fsharp'"> | ||
<PropertyGroup> | ||
<IsTestProject>true</IsTestProject> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you force false directly on new project coverlet.tests.projectsample.fsharp.fsproj
?
Or in future we need to add every time projects here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes sorry didn't think about this. I was getting a error when dotnet test
the fsharp project and I didn't know why.
Took me a good amount of time to figure out it is the Directory.Build.pros
. The other c# class library projects don't have this issue even when <IsTestProject>true</IsTestProject>
. Seems to be a fsharp issue.
using Moq; | ||
using Coverlet.Core.Abstractions; | ||
|
||
namespace Coverlet.Core.Helpers.Tests | ||
{ | ||
public class InstrumentationHelperTests | ||
{ | ||
private readonly InstrumentationHelper _instrumentationHelper = | ||
private InstrumentationHelper _instrumentationHelper = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer keep this readonly
and define new one only for that test wdym?
Like you did in CanInstrumentFSharpAssemblyWithAnonymousRecord
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
# Conflicts: # Documentation/Changelog.md
closes #1145