Cleanup logs after successful test runs #40411
Conversation
dougbu
requested review from
BrennanConroy,
Pilchie,
Tratcher and
halter73
as code owners
There was a problem hiding this comment.
while driving by I found these <Content/> items were fading into the <Compile/> items
Pilchie
added
the
area-infrastructure
|
Working to address a couple of concerns:
Will comment when I mark this PR ready for review again. |
I prefer this. Sometimes it's nice to look at similar passing tests to see if there are patterns in the logs. I hope most test assemblies pass at a high enough rate that we shouldn't bother optimizing the artifact size when they fail. It'd really be nice if I could also look at logs from passing tests in other assemblies when there's a failure. Can we only clean logs if the entire test matrix succeeds? When I'm debugging rare ci-only test failures, I want to keep all the clues available to me. |
@halter73 the direction I was heading before your comment was to keep all logs created by I could leave things as-is, meaning (switching from InMemory.FunctionalTests to Kestrel.Core.Tests) a failure in Note I can't keep logs for everything if tests in some other assembly or for a different TFM in the same test project fail. Our Helix work items are per-test assembly aka per-test project / TFM pair. |
There was a problem hiding this comment.
This is specific to AssemblyTestLog currently but is a pattern other classes could follow. Adding a default constructor to AssemblyTestLog wouldn't be a good idea.
|
Ready for review |
|
This does a great deal of cleanup compared to the before picture, especially when looking at non-Windows work items (none of which create more than 5 jobs). I may look into the IIS logging but that doesn't appear urgent. BeforeBuild #20220228.5 of main.
AfterBuild #20220227.10 of this PR.
For posteritySample query |
|
Hmm, have some room to improvement on the space our files consume.
|
|
Oops, that was for the build of 'main'. The after story (again, on Windows) is
|
src/Testing/src/AssemblyTestLog.cs
Outdated
There was a problem hiding this comment.
While I get what this is doing, the check isn't really needed and sort of makes it confusing when reading.
I would just always set the state, change the function name to something like "MarkHasTestFailure" or "RecordHasTestFailure", and if you really like the check maybe throw an exception.
There was a problem hiding this comment.
Glad you pointed me here @BrennanConroy. My intention was to ensure we didn't lose track of reported test failures because another class requested cleanup later. This line means problems reported before the first cleanup request are ignored
Just updating _cleanupState is more correct but insufficient because other AssemblyTestLog users in the same assembly might not inherit from LoggedTestBase. I need to ensure all users report their failures if any users exist in the assembly because the cleanup has to be all or nothing.
There was a problem hiding this comment.
I am working on two options here:
- Revert changes to
LoggedTestBaseand haveAspNetTestAssemblyRunnersomehow notifyAssemblyTestLogof test failures. This would entirely eliminate the concept of registering for cleanup and would mean any failure in a test assembly prevents cleanup. - Update all
AssemblyTestLogusers to make sure they notify that class of test failures. Basically this option would make not registering fail loudly and longly. If I go this route, I'll use @halter73's two-boolsuggestion.
Is this where we landed? As long as all the logs from the same helix job as the failing test are kept it's fine. I mostly like keeping the logs from other tests to find evidence of thread pool starvation, ephemeral port exhaustion or other similar environmental issues that can cause a bunch of tests in the same job to fail. |
That was my proposal. I'm asking if it's enough for you. I should have mentioned: Because Helix work items are per-assembly i.e. test a single TFM from a single test project, you'll get all of the logs for the work item. For something like ServerComparison.FunctionalTests, a single test failure won't impact logging for other tests sharing the same build agent but you'll get all of the logs for that assembly. |
src/Testing/src/AssemblyTestLog.cs
Outdated
There was a problem hiding this comment.
Glad you pointed me here @BrennanConroy. My intention was to ensure we didn't lose track of reported test failures because another class requested cleanup later. This line means problems reported before the first cleanup request are ignored
Just updating _cleanupState is more correct but insufficient because other AssemblyTestLog users in the same assembly might not inherit from LoggedTestBase. I need to ensure all users report their failures if any users exist in the assembly because the cleanup has to be all or nothing.
src/Testing/src/AssemblyTestLog.cs
Outdated
There was a problem hiding this comment.
Thinking about multiple test classes executing their tests in parallel, should this be bullet-proof e.g. using Interlocked.CompareExchange(...) (and a full-sized int enum)❔
There was a problem hiding this comment.
You could always lock in ReportTestFailure. Although per-instance lock might be better than the static _lock. I think the best options is to avoid locking or interlocking entirely by turning _cleanupState into two bools: bool _wasTestFailure and bool _cleanupRequested.
Based on my reading, neither of these bools could ever transition from true to false, so there's no reason for any comparisons before setting either of these to true. Dispose can then delete the directory only if _cleanupRequested && !_wasTestFailure.
There was a problem hiding this comment.
That'll work @halter73. I merged them into an enum primarily to make updates atomic but that didn't work out very well 😉
There was a problem hiding this comment.
I'm requesting re-reviews because this iteration is fairly different from what was in this PR before. A lot of that's in testing of Microsoft.AspNetCore.Testing but the src/ change to have AspNetTestAssemblyRunner handle failure reporting to AssemblyTestLog is significant too.
There was a problem hiding this comment.
This is important because individual tests don't own AssemblyTestLog instances, AspNetTestAssemblyRunner does
dougbu
requested review from
BrennanConroy,
halter73 and
wtgodbe
and removed request for
pranavkm
src/Testing/test/TestableAssembly.cs
Outdated
|
@halter73 @BrennanConroy who knows logging fairly well❔ I added a bunch o' logging to figure out why Problem is specific to Linux and OSX and reproduces under WSL2. Any suggestions❔ |
|
Below is from the console log of recent failure on Helix OSX agent, showing both the settings for Microsoft.AspNetCore.Testing, the What's worth checking next❔ |
|
So, I finally checked if |
- #39038 - update `AssemblyTestLog` to perform actual log directory cleanup - add `ReportTestFailure()` method for tests to report failures, disabling cleanup - add `IAcceptFailureReports` for `AspNetTestAssemblyRunner` to report failures to `AssemblyTestLog` - extend `AspNetTestAssemblyRunner` to optionally create fixture instances using `static ForAssembly(Assembly)` - add `AssemblyTestLogFixtureAttribute` to register `AssemblyTestLog` as an assembly fixture - use `AssemblyTestLogFixtureAttribute` in all test projects - disable log cleanup in three existing tests nits: - use `is [not] null` and `new()` more
- also cover a few existing methods nit: move `AssemblyTestLogTests` to same namespace as `AssemblyTestLog`
- was deleting the just-created global.log file
|
🎉 Found the problem 🎉 Please review the last commit if you're interested in the details. I'd also appreciate any comments before I squish 'n merge this sometime tomorrow… |
| var baseDirectory = TestFileOutputContext.GetOutputDirectory(assembly); | ||
| var stackTrace = Environment.StackTrace; | ||
| if (!stackTrace.Contains( | ||
| "Microsoft.AspNetCore.Testing" |
There was a problem hiding this comment.
Is this ever false? Wouldn't it include the current method which is in Microsoft.AspNetCore.Testing.
There was a problem hiding this comment.
Thought it might be but you're probably right. I'll play with this in a follow-up when I get a bit of time.
ghost
added this to the
|
/backport to release/6.0 |
|
Started backporting to release/6.0: https://github.com/dotnet/aspnetcore/actions/runs/2017735060 |
|
@dougbu backporting to release/6.0 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Cleanup logs after successful test runs - #39038 - update `AssemblyTestLog` to perform actual log directory cleanup - add `ReportTestFailure()` method for tests to report failures, disabling cleanup - add `IAcceptFailureReports` for `AspNetTestAssemblyRunner` to report failures to `AssemblyTestLog` - extend `AspNetTestAssemblyRunner` to optionally create fixture instances using `static ForAssembly(Assembly)` - add `AssemblyTestLogFixtureAttribute` to register `AssemblyTestLog` as an assembly fixture - use `AssemblyTestLogFixtureAttribute` in all test projects - disable log cleanup in three existing tests
Using index info to reconstruct a base tree...
M src/Mvc/test/Mvc.FunctionalTests/ErrorPageTests.cs
M src/Testing/src/AssemblyTestLog.cs
M src/Testing/src/xunit/AspNetTestAssemblyRunner.cs
M src/Testing/test/AssemblyTestLogTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Testing/test/AssemblyTestLogTests.cs
CONFLICT (content): Merge conflict in src/Testing/test/AssemblyTestLogTests.cs
Auto-merging src/Testing/src/xunit/AspNetTestAssemblyRunner.cs
CONFLICT (content): Merge conflict in src/Testing/src/xunit/AspNetTestAssemblyRunner.cs
Auto-merging src/Testing/src/AssemblyTestLog.cs
CONFLICT (content): Merge conflict in src/Testing/src/AssemblyTestLog.cs
Auto-merging src/Mvc/test/Mvc.FunctionalTests/ErrorPageTests.cs
CONFLICT (content): Merge conflict in src/Mvc/test/Mvc.FunctionalTests/ErrorPageTests.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Cleanup logs after successful test runs - #39038 - update `AssemblyTestLog` to perform actual log directory cleanup - add `ReportTestFailure()` method for tests to report failures, disabling cleanup - add `IAcceptFailureReports` for `AspNetTestAssemblyRunner` to report failures to `AssemblyTestLog` - extend `AspNetTestAssemblyRunner` to optionally create fixture instances using `static ForAssembly(Assembly)` - add `AssemblyTestLogFixtureAttribute` to register `AssemblyTestLog` as an assembly fixture - use `AssemblyTestLogFixtureAttribute` in all test projects - disable log cleanup in three existing tests
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
5 participants
LoggedTestCleanerFixtureAttributeto help track test failuresLoggedTestCleanerto perform actual log directory CleanupAssemblyFixtureAttributeandAspNetTestAssemblyRunnerto support communication between the aboveLoggedTestCleanerFixtureAttributein test projects that produce large numbers of usually-useless log files