Add ability to instrument non-referenced assemblies #213
Conversation
…roject references can also be instrumented.
Codecov Report
@@ Coverage Diff @@
## master #213 +/- ##
==========================================
- Coverage 94.91% 94.39% -0.53%
==========================================
Files 15 16 +1
Lines 1633 1765 +132
==========================================
+ Hits 1550 1666 +116
- Misses 83 99 +16
Continue to review full report at Codecov.
|
|
Looks like PR #204 also deals with the merging of files with the same name and different paths. |
…ple locations is already supported in the master branch.
|
@pape77 That's a nicer solution, so I've reverted the changes I made to CoverageResult.cs in my PR. |
|
@jherby2k nice :) Have you retested with this? |
|
Of course! It works fine.
|
|
I'm happy to update the readme as well, I just want to confirm this looks good first. |
|
@pape77 on further testing it does not work fine :(. It merges 2 copies of the same file no problem, but it can't instrument 2 different files with the same name at the same time. So I've reverted to my method, which uses the full path as key and then merges them together at the end if the names match. A bit more complicated, but more robust too. |
|
@jherby2k I couldn't quite follow. In the example of my pr i do a dotnet test on a solution that has several test projects. |
|
It’s specific to my PR’s changes. With the IncludeDirectories option, you might end up with two copies of the same file during the same run. Really only one gets loaded into memory, but since you don’t know which will get loaded first, they need to be instrumented separately (so two different keys in the dictionary) and then merged before displaying the results. So my PR always does a merge at the end, and merges any files with the same name.
|
|
@jherby2k kindly fix the merge conflicts so I can review. Thanks! |
|
Still have some questions regarding merging results but will have a clearer picture once I can make a more wholesome review. Apologies for letting this slide for too long |
|
@tonerdo No problem! Conflicts resolved. Sorry about all the commits - I had some issues with line endings in Coverage.cs. |
|
@jherby2k not to worry. I'm trying to wrap my head around your merging logic and trying to figure out if it's indeed needed. Will take a much deeper look after that |
|
If one of my libraries is a dependency of multiple add-ins/ extensions, it will appear in multiple folders at once, right? And each copy gets instrumented during testing. The merging combines them into a single result. If I don’t merge the result, the dependency gets listed multiple times.
Hope that makes sense.
|
|
@jherby2k just a heads up that this is still on my radar |
|
@jherby2k Please see my PR #253 which deals with the same issue. Unfortunately I overlooked your PR. If you agree I will incorporate the changes I made in the InstrumentationHelper class into your PR. There's only one problem. I wanted to avoid having duplicate assembly names at all as that could leave to different issues. My solution is to prioritize the assembly probing:
That allows us in corefx i.e. to use the IL version of System.Private.CoreLib.dll. Means, I need a way to load assembly a from directory A but not load assembly a from directory B. |
|
Hi @jherby2k, what @ViktorHofer said is something I've been thinking about. Instead of trying to merge results of same name files which IMO could be quite error prone. How about we only add an assembly with a specific name once and ignore others even though they might be in different locations? |
|
@ViktorHofer and @tonerdo
I'm cool with selecting only one in principal, but in practice the above order isn't necessarily what I want. For example, I have a binary PowerShell module that depends on and returns types defined in "common.dll". The module exists in its own directory. The test assembly also references "common.dll" because it tests the results returned from that module. But I want the one in the module directory instrumented, not the one in the test directory. |
|
How about a MergeIdenticalFileNames switch? Too esoteric? |
|
Having assemblies with the same assembly and module name isn't support well as it comes with different issues outside of the GAC world. I strongly recommend not going that path... |
|
Poorly named option then - not what I intended. How about MergeIdenticalAssemblies? With an additional check to see if the assemblies really are identical. So, if MergeIdenticalAssemblies is enabled, and the assemblies have the same name and are identical, my behavior wins. Otherwise, your behavior. |
Can you please explain what identical assemblies are and what they are for? Why do I need to instrument identical assemblies multiple times? |
|
you don't. As you said, you just need to instrument one of them. But your intended probe order is the opposite of mine. That's why I just instrument them both and then merge the results. One will have 0 hits, one will have the results you want. It doesn't matter which is which and what the probe order is. |
|
But instrumenting both takes a considerable amount of time and doesn't seem right. If your issue is the priority, why don't we introduce an option to control it? |
I still don't understand why the order matters if they are identical? (Just wanna understand your case better, no offense!) |
|
You might be right, and your solution may do the same thing for me. Let me test yours when I have a bit of time. |
|
Sure, let me know if I can help somehow. And sorry to be so pushy to get this feature in but we depend on it in corefx ⚡️ |
* Added TeamCityOutput command line flag for printing TeamCity coverage service messages * Added unit tests for TeamCityServiceMessageWriterTests * README updates * Added teamcity reporter * Removing teamcity output command line param * README updates * Cyclomatic complexity computation added Signed-off-by: Norbert Žofák <nzofak@protank2.onmicrosoft.com> * Changed UseConsoleOutput boolean to ReporterOutputType enum * Merging TeamCityServiceMessageWriter with TeamCityReporter * bump version numbers * 243 - Swapping percentage with total lines for proper TeamCity statistics * Allow relative output path for console runner * Apply relative output path fix to msbuild task as well * Fix bug in the performance test and document how to run it in README.md. * fix description for include [*] filters * Updated Mono.Cecil to fix hang when writing modules. * Use Windows-style parameters to be consistent with other command examples * Added an "IncludeDirectories" param so that assemblies that are not project references can also be instrumented. * Remove WriteLine I accidentally left behind. * Revert changes to CoverageResult, as merging the same file from multiple locations is already supported in the master branch. * Go back to full module paths as keys - it wasn't working in all scenarios. * Skip included directories that don't exist quietly, rather than throwing. * Restore accidentally-removed #220. * Apply PR from viktorhofer
…nto include-path
|
@ViktorHofer Sorry about the unnecessary back and forth - looks like your solution gives the results I want regardless. I've merged your PR and added one extra commit to revert my now-unnecessary backup file naming algorithm. |
|
Your PR seems wrong as it contains all the commits that happened since you opened yours. You probably want to fix that by rebasing or merging or just take my PR where I already did that: #253 |
|
Blargh, I give up. I'll add my one change to #253. |
As discussed in #192, I want to be able to instrument assemblies that aren't project references (loaded at runtime). I actually have 2 use cases:
To accomplish this, I added an "IncludeDirectories" parameter which allows additional paths to be searched for modules. It supports relative and absolute paths, and a wildcard (*) for including one level of subdirectories (useful for MEF extensions).
One side effect is the possibility that one or more assemblies exists in multiple locations. I dealt with this by including a hash of the file path in each backup file name, and then merging the results for all files with the same name after instrumentation.
Example that includes both a bunch of extension subdirectories and the PowerShell.Commands module directory (both relative paths from the project output directory):
dotnet test --no-build /p:CollectCoverage=true /p:IncludeDirectories="Extensions\*,PowerShell.Commands"