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

Pipelines are seen as classes in cobertura report #177

Closed
MatthijsPrent opened this issue Jan 12, 2023 · 5 comments
Closed

Pipelines are seen as classes in cobertura report #177

MatthijsPrent opened this issue Jan 12, 2023 · 5 comments
Labels

Comments

@MatthijsPrent
Copy link

Hello Steve,

First of all, thank you for creating and maintaining AltCover, it is certainly appreciated!

On our buildserver I would like to make the switch from OpenCover to AltCover, however I ran into a problem. I am testing a solution with multiple projects on a jenkins buildserver, we would like to generate a Cobertura test report that can easily be published through Jenkins. However, in the coberturea report, in some cases pipelines are seen as different classes, which would generate an incorrect report.

Because our main project contains some IP that I may not share, I have created a simple demo project to demonstrate my problem, it can be found at:
https://github.com/MatthijsPrent/PipesAsClassesBug
This solution contains a very simple library called "PipesAsClassesBug" and a corresponding test project. The tests inside of the project are not very well implemented, but that is also not the purpose of this example solution, they should be sufficient for this example.
The repository also contains a test.bat file. This file contains the command that I use to run AltCover, and a sed command that I use before publishing the data on the buildserver, to remove absolute paths.

After running this bat, the output file can be found ad PipesAsClassesBug/PipesAsClassesBug.Test/AltCoverCobertura.xml. For example at line 168 the class "PipesAsClassesBug.SomeMethods/Pipe #1 stage #1 at line 13@13" is documented. However, this is not a class, but simply a F# pipe. Therefore, I would expect it not to be reported as a class.

I would like to ask you to look at my issue and provide me with some advice on how I can get the reporting correct. I hope that my example and explaination is clear, if it is not, please don't hesitate to ask me any questions. Thanks in advance for your reaction.

Kind Regards,
Matthijs Prent

@SteveGilham
Copy link
Owner

SteveGilham commented Jan 12, 2023

Decompiling the assembly PipesAsClassesBug.dll reveals that class PipesAsClassesBug.SomeMethods.Pipe #1 stage #1 at line 13@13 looks like this

internal sealed class Pipe #1 stage #1 at line 13@13 : FSharpFunc<ParameterInfo, string>
{
	internal static readonly Pipe #1 stage #1 at line 13@13 @_instance = new Pipe #1 stage #1 at line 13@13();

	[CompilerGenerated]
	[DebuggerNonUserCode]
	internal Pipe #1 stage #1 at line 13@13()
	{
	}

	public override string Invoke(ParameterInfo paraminfo)
	{
		return paraminfo.Name + paraminfo.ParameterType.Name;
	}
}

and in fact represents the function object being passed to the Seq.map operation. The elaborate and somewhat misleading name is an artefact of compiler changes in the last year or two. Older compiler versions used simply to name the type according to the containing function, as, in this case, PipesAsClassesBug.SomeMethods.containsParameter@13.

The interesting thing in this example is that the function object representing the predicate on line 22 of the same code is still (using the 7.0.102 SDK to build) named according to the older convention as PipesAsClassesBug.SomeMethods.getMethodsWithAttributeInType@22-1.

I have not bothered to delve into the guts of the compiler to see the whys and wherefores of the change.

@MatthijsPrent
Copy link
Author

Thank you for your quick response and for the explaination. It indeed makes sense that if the decompilation shows a class, that a coverage tool would not be able to recognize that it actually is not a class.

However that leaves me wondering why OpenCover does succeed in not marking it as a class. Is this an additional feature, that might come to AltCover as well in the future?

@SteveGilham
Copy link
Owner

SteveGilham commented Jan 13, 2023

I performed the following experiment to see what OpenCover is doing.

1 - create new F# console project (VS 17.4.4)
2 - edit target framework to be net472
3 - paste in the content of the PipesAsClassesBug source file Library.fs to overwrite the generated stub
4 - append the following main program entry point


    [<EntryPoint>]
    let private main arguments =
      let methodInfo = containsParameter.GetType().GetMethods()
      let result = containsParameter "" "" methodInfo.[0]
      printfn "result = %A" result
      0

5 - build
6 - run under opencover v4.7.1221, generating the attached
results.zip

Loading the results.xml as an XML document in powershell via $x = [xml](get-content .\results.xml), the class manifest can be dumped

image

containing the same Pipe-named function object type as before (the line numbers on the @ types differ as the code formatter I use in Visual Studio has collapsed the double blank at lines 9-10 of the original to just one).

By way of comparison, the AltCover emulation of OpenCover generates the following manifest

image

omitting the generated types that contain no methods.


From what you are saying, though, I get the impression that emulating OpenCover's warts-and-all reporting style is not actually what you want. In that case, the coverlet emulation (a style that collapses those implementation detail types like PipesAsClassesBug.SomeMethods/aux@41 into the surrounding user top level function) should do what you want. To do this, just add /p:AltCoverReportFormat=Json to the dotnet test command line to change the intermediate report format. The Cobertura report generated in this fashion looks like this -

coverage.zip

@SteveGilham SteveGilham added question ready to close Believed addressed, waiting to hear to the contrary labels Jan 18, 2023
@MatthijsPrent
Copy link
Author

Thank you for the extensive response. Unfortunately I haven't had time to fully reproduce and investigate it.

However, I have ran some quick tests with /p:AltCoverReportFormat=Json and it indeed seems to do more what we want. I will experiment with it more in the coming weeks, but for now it looks like what I want!

Thank you.

@SteveGilham
Copy link
Owner

Closing as the change in report format seems to have alleviated the original concerns.

@SteveGilham SteveGilham removed the ready to close Believed addressed, waiting to hear to the contrary label Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants