New parameter to control automatic assembly exclusion #1392
Conversation
There was a problem hiding this comment.
Thanks for having a go at this, @daveMueller! Sorry I didn't have time to review it until now.
It looks good, and I agree with changing the default behaviour and removing the old heuristics for generated code. Might that require a major version bump, since it might change the behaviour of instrumentation for existing projects?
I only have some minor suggestions for code and logging tweaks, and the documentation seems inverted.
Documentation/MSBuildIntegration.md
Outdated
| | MissingAny | Includes the assembly if at least one document is matching. In case the `ExcludeAssembliesWithoutSources` parameter is not specified the default value is `MissingAny`. | | ||
| | MissingAll | Includes the assembly only if all documents can be matched to corresponding source files. | |
There was a problem hiding this comment.
The parameters got mixed up in the documentation, this seems to be the intended meaning:
| | MissingAny | Includes the assembly if at least one document is matching. In case the `ExcludeAssembliesWithoutSources` parameter is not specified the default value is `MissingAny`. | | |
| | MissingAll | Includes the assembly only if all documents can be matched to corresponding source files. | | |
| | MissingAll | Includes the assembly if at least one document is matching. In case the `ExcludeAssembliesWithoutSources` parameter is not specified the default value is `MissingAll`. | | |
| | MissingAny | Includes the assembly only if all documents can be matched to corresponding source files. | |
| { | ||
| return (false, docName); | ||
| _logger.LogVerbose($"Unable to instrument module: {module}, pdb without any local source files"); |
There was a problem hiding this comment.
| _logger.LogVerbose($"Unable to instrument module: {module}, pdb without any local source files"); | |
| _logger.LogVerbose($"Excluding module from instrumentation: {module}, pdb without any local source files"); |
| if (!allDocumentsMatch) | ||
| { | ||
| _logger.LogVerbose( | ||
| $"Unable to instrument module: {module}, pdb without local source files, [{FileSystem.EscapeFileName(notFoundDocument)}]"); |
There was a problem hiding this comment.
| $"Unable to instrument module: {module}, pdb without local source files, [{FileSystem.EscapeFileName(notFoundDocument)}]"); | |
| $"Excluding module from instrumentation: {module}, pdb without local source files, [{FileSystem.EscapeFileName(notFoundDocument)}]"); |
| var documentSourceMap = DocumentSourceMap(metadataReader).ToList(); | ||
| return documentSourceMap.Any(x => x.documentExists.Equals(true)); |
There was a problem hiding this comment.
Minor detail, this could work on the enumeration directly, but at the price of making debugging harder:
| var documentSourceMap = DocumentSourceMap(metadataReader).ToList(); | |
| return documentSourceMap.Any(x => x.documentExists.Equals(true)); | |
| return DocumentSourceMap(metadataReader).Any(x => x.documentExists); |
|
|
||
| private IEnumerable<(string documentName, bool documentExists)> DocumentSourceMap(MetadataReader metadataReader) | ||
| { | ||
| return metadataReader.Documents.ToList().Select(docHandle => |
There was a problem hiding this comment.
Is it necessary to do the ToList() here?
| var documentSourceMap = DocumentSourceMap(metadataReader).ToList(); | ||
|
|
||
| if (documentSourceMap.Any(x => x.documentExists.Equals(false))) | ||
| return (false, documentSourceMap.FirstOrDefault(x => x.documentExists.Equals(false)).documentName); |
There was a problem hiding this comment.
Same as above, can work on the enumerable:
| var documentSourceMap = DocumentSourceMap(metadataReader).ToList(); | |
| if (documentSourceMap.Any(x => x.documentExists.Equals(false))) | |
| return (false, documentSourceMap.FirstOrDefault(x => x.documentExists.Equals(false)).documentName); | |
| var missingDoc = DocumentSourceMap(metadataReader).FirstOrDefault(x => !x.documentExists); | |
| if (missingDoc != null) | |
| { | |
| return (false, missingDoc.documentName); | |
| } |
There was a problem hiding this comment.
I'm not so sure about this one. FirstOrDefault isn't meant to check whether a value exists or not, although we all (ab)use it this way. In this case FirstOrDefault would return a ValueTuple<string, bool> which defaults to (documentName: null, documentExists: false).
I now could adapt it to sth. like if (missingDoc.documenName != null){...} but now it somehow reads like: "If there is a document whose name can't be resolved lets include the assembly."
Even tho' this is just because all documents have a matching source file.
There was a problem hiding this comment.
I missed the tuple data type here, so ignore this comment :)
There was a problem hiding this comment.
simplified the predicate
| } | ||
|
|
||
| [Fact] | ||
| public void EmbeddedPortablePDPHasLocalSource_AllDocumentsExist_ReturnsTrue() | ||
| public void EmbeddedPortablePDPWithAssemblySearchTypeMissingAny_AllDocumentsExist_ReturnsTrue() |
There was a problem hiding this comment.
Since both MissingAny and MissingAll are tested:
| public void EmbeddedPortablePDPWithAssemblySearchTypeMissingAny_AllDocumentsExist_ReturnsTrue() | |
| public void EmbeddedPortablePDPHasLocalSource_AllDocumentsExist_ReturnsTrue() |
It would also be good with a testcase where one file is missing, to test the difference in behaviour betweeen the two cases.
There was a problem hiding this comment.
added test with one missing document
|
Thanks for the review @petli 🙏 . I'll have a look in the next days. |
|
Just a note that we shouldn't merge this before a minor fix with #1395 has been released, so people don't have to onboard the new behaviour at the same time. |
|
@daveMueller we merged this one in past(released on 3.2.0) #1360 does this PR invalidate that one right? I think we should add a new document where we list the breaking changes between versions so we can point the user there in case of questions, wdym? We can after link to the main readme as a new section. |
Yes you are right. The parameter from #1360 was replaced by setting the Maybe we could add/mark the breaking changes in our changelog like other projects are doing it, e.g. https://github.com/cake-build/cake/releases/tag/v2.0.0. Or were you thinking about sth. like a table that lists the different versions and their breaking changes? |
|
I personally would prefer to align the description of the parameter values and make them lower case in these two documentations? Something like coverlet/Documentation/VSTestIntegration.md Line 123 in 70fe319 coverlet/Documentation/MSBuildIntegration.md Lines 237 to 241 in 70fe319 |
It's fine to me, I was wondering if we have around other BC for the next version, pls bump the version inside the version.json and coverlet.core.csproj like here https://github.com/coverlet-coverage/coverlet/blob/master/Documentation/ReleasePlan.md#how-to-manually-release-packages-to-nugetorg in this PR. I remember some issue with the default msbuild property like /Include etc... and the idea to change to |
I thought about this again and decided to leave it like it currently is ( |
Co-authored-by: Peter Liljenberg <peter@klavrekod.se>
|
Thanks @petli @daveMueller going to merge! |
closes #1164
There are two things I wasn't really sure about.
I aligned the parameter description to the description in the feature #1164. But in the rest of the documentation the
values for the other parameters are always lowercased. So I would suggest to also describe them in the documentation
as
missingall,missingany,none.In the actual code it doesn't matter because it is compared case insensitive.
The second thing is that in the feature description it says that
MissingAnyshould be the current behavior. Personally Ithink that after changing the default behavior of the heuristic to
MissingAll, the exclusion of generated document namesand the exclusion of the
unknownF-sharp module don't make much sense anymore. Thus I removed the functionsIsGeneratedDocumentNameandIsUnknownModuleInFSharpAssemblyfrom theInstrumentationHelper.But I can also add them back again?