-
Notifications
You must be signed in to change notification settings - Fork 19
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
.NET 5 issues? #105
Comments
New compiler versions, new IL -- I'm not entirely surprised by this, as the call tracking -- effectively wrapping the indicated method in a A simple repro would be much appreciated, as none of the previous awkward cases I've kept as regression testing for this class of error showed any issues with the .net 5 build (though some of the other parts of the test suite showed changes due to the IL being generated being different). |
The dilemma is that it's a client code base that I can't share with anyone - I'm under NDA. However, it's using NUnit if that's any help? |
Hmmm, in addition, it works just fine for tests that follow one of the following conditions:
The failing tests have none of the above conditions, and are all async tasks. So perhaps it has something to do with async tests? |
FWIW, in my LLVM to CIL project, I am finding that .NET 5 is much stricter about IL. There are places I was generating IL that wasn't quite right and .NET Core 3.1 would let me get away with it, but .NET 5 does not. I'm just starting to work through these problems so I don't have more specifics yet. |
Async tests is a possibility -- that's not something I've played with. Cutting down a known-bad test to just its essentials (of which the system under test is not one) and seeing what does and doesn't work would at least make some progress towards having a clean repro case. |
Yes that's it -- this is enough to provoke the IPE
|
Also, thinking about it, the use of
|
Can an AsyncLocal help here? |
AsyncLocal could be part of a solution; and that would be the point at which I am forced to finally abandon CLR2 support (which would simplify things elsewhere). Still not sure how to say when an async test has finished, though. At the moment, I'm still trying to figure out what's wrong with the IL, as PEVerify isn't giving any hints -- both the original and instrumented giving |
Not sure if it can be used as an inspiration (and not sure if there is .NET 5 support in it), but Minicover (https://github.com/lucaslorentz/minicover) has a very simple and readable implementation, and I think they support async code. Altcover is obviously superior, but perhaps we could get inspired by how they are solving it? |
Now I see what's wrong -- decompiling the instrumented code with ILSpy pops up a "Stack underflow" error at the end; the As for async in general, I have no problem simply instrumenting async code for visiting, although so far I do nothing by way of hiding the branches injected by the state machine implementation (see e.g. issue #97). The problem arises for call tracking, as now the time to stop tracking against that test is when the Task completes, and not simply when the test method returns. When decompiled, the unit test above looks like
and when instrumented for call context
|
I wonder how Minicover deals with this. They have a test for async code that works I think. Could that help? |
But if I'm reading this correctly, I also suppose net5 is not the issue, but async/await in general? |
See here for the IL they generate for async methods https://github.com/lucaslorentz/minicover/blob/master/tests/MiniCover.UnitTests/Instrumentation/AsyncMethod.cs |
That could be useful, but not for the reason that MiniCover instruments async methods. What is useful is that the IL shows that MiniCover approaches the instrumentation via a per-method context object (created via a call to By using the try/finally approach everywhere, that means MiniCover has some robust code for wrapping methods of any signature like that, whereas AltCover only uses a try/finally wrapper for It won't resolve the problem of the Task object's completion not being at the same point in time as it being returned, or the current use of thread local context storage. In detail whereas MiniCover's recording a visit looks like
AltCover just invokes a static method with the assembly ID and an assembly-level index
Instrumenting the
As this one doesn't have any branching within it, nor any |
The secret sauce actually appears to be inside |
Correction, it's a MiniCover extension method, rather than part of Cecil, so I shall have to port it rather than just add an invocation. Still, it's well encapsulated, which makes things simple enough. |
Exciting stuff! Seems like you're headed in the right direction. Thanks for sharing all this so extensively. Let me know if I can be of assistance. By the way, I accepted your Skype invitation. |
The InvalidProgramException should be fixed in the latest pre-release GitHub build; as I had hoped, it was a case of just adding a dozen lines of code to handle non-void returns. That is enough to show some of the limitations mentioned above -- from actual data gathered from the same test case as before
where we would expect that the whole method would be tracked. I have strategies for resolving those issues, so I'll be in coding haze for probably another couple of days before I come up for air and take the Skype off the hook. |
Alright, sounds great! Keep me posted @SteveGilham - you're a champ. Thanks to your product, I can release 1.0 of mine soon as well! |
It's great that you're making branches for each feature (https://github.com/SteveGilham/altcover/compare/develop/github/issue-105). Fun to follow along. Again, let me know if there's anything I can do to assist or help. Can't wait until this is released! |
Woah, that was quick for such a crazy feature! 😳 Is it on nuget? |
Or perhaps there's even a private NuGet server somewhere where it can be fetched? I'd love to test it out already tonight. |
My apologies. I see that the artifacts link is private, as well as being obscure -- I'd expected it to be obscure, but public, like the AppVeyor ones are. As I'm wrapping up #106 at the moment, I expect to release properly tomorrow (assuming no more enhancement requests). |
No worries, and perfect with tomorrow's release! 🙏 |
Should be resolved in release 7.2.800 |
Actually, here's a version with some more logging as well (in case of something truly weird going on) |
Hah, okay. I just tested the other one. I saw your comment pop up live :D |
I'll check with the latest one. |
Here's the latest logs (truncated at beginning):
|
Thanks. Now I understand. It's a bug that affects the async Expect a fix soon. |
Awesome! Looking forward to it 😍 Any chance it'll make it by Sunday? |
Here's another private build, this time from GitHub Actions CI. This should resolve the issue; if as expected it is good, I can push out another full release pretty much straight away. |
I'll hopefully get time to try it Sunday! 🙏 |
Trying it now! |
New error:
|
Now this looks like an artifact of double instrumentation -- project A gets instrumented and the recorder gets added to the JSON dependency manifest, then project B which depends on project A gets built (before the instrumentation gets rolled back), and rolls project A's manifest into its own, and then gets instrumented again, and is failing when the recorder gets added a second time. That has been a bug waiting to happen (though clearly in a low frequency use case) since I added .net core support nearly 3 years ago. |
What do I do about it though? Is it an easy fix? |
I thought this would be an easy repro & fix but I see I already have a unit test for the scenario I thought this was, and a couple of obvious variations on the current tests also ran clean. Could I trouble you for the full stack trace? |
Do you mean a larger portion of the log dump, or? How do I retrieve this full stack trace? |
The one from the |
No worries at all - coming right up! |
Here it is!
|
Speculative fix (private build again) |
That worked! Everything seems to be operational. Great job! |
Any ideas when we can expect it to release to NuGet? |
In the next hour or so. |
Thank you - that's awesome! <3 |
Disclaimer: Not fully sure this is due to .NET 5.
But after using .NET 5 and adding AltCover, I get the following error in all my tests:
The text was updated successfully, but these errors were encountered: