-
-
Notifications
You must be signed in to change notification settings - Fork 245
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
Share container lifetime implementation across all container tests #909
Conversation
✅ Deploy Preview for testcontainers-dotnet ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
I don't understand what's happening with the Run Tests step timing out after 6 hours. I downloaded the logs and parsed them with this quick and dirty code. var logFile = new FileInfo(@"logs_2950\build (ubuntu-22.04)\8_Run Tests.txt");
using var logStream = logFile.OpenRead();
using var logReader = new StreamReader(logStream);
var results = new List<TestResult>();
while (logReader.ReadLine() is {} line)
{
if (line.Contains("Passed:"))
{
var status = line.Substring(29, line.IndexOf('!') - 29);
var durationStart = line.IndexOf("Duration: ") + 10;
var duration = line.Substring(durationStart, line.LastIndexOf(" - ") - durationStart);
var assembly = line.Substring(line.IndexOf("net6.0/") + 7);
results.Add(new TestResult(status, duration, assembly.Substring(0, assembly.Length - 9)));
}
}
results.OrderBy(e => e.Assembly).Dump();
public record TestResult(string Status, string Duration, string Assembly); And here are the results.
There are 29 assemblies that were successfully tested. The one missing (
The tests passed successfully in #908 so maybe they will pass if #908 gets merged first? |
I have not looked into the PR yet, I will review both PRs later today, but typically, this situation occurs when a wait strategy continuously fails. Currently, there is no default timeout or maximum number of retries in place. This is something I am currently working on. After looking into the logs this seams not the case for this pr 🤔. I need to take a closer look into it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not entirely sure what the root cause of the issue is, but the changes in Testcontainers.Commons
are causing it:
The one missing (
Testcontainers.Commons.dll
) targets .NET Standard and doesn't contain any tests.
Although the logs indicate that no tests are being executed, the process actually hangs. At least, that is what happens on my machine when I run dotnet test
within tests\Testcontainers.Commons
directory. When I change the target framework to net6.0
it works fine.
By setting the `IsTestProject` property to false in Testcontainers.Commons.csproj Cause: referencing the `xunit` package enables `IsTestProject=true` which trigger a bug in vstest where [netstandard libraries hangs indefinitely][1]. This bug was addressed 5 days ago: [Fix execution gets stucks on single netstandard source][2] [1]: microsoft/vstest#4392 [2]: microsoft/vstest#4497
Thanks for your input. I now understand what's happening: Referencing the I have addressed this by explicitly setting the |
Here applies the same as for #908. We do not want to increase coupling in our module tests. I will close the PR for now. If you have any other thoughts on that topic, please consider reopening it again. |
What does this PR do?
This PR makes all container test classes inherit from a new abstract
ContainerTest<TBuilderEntity, TContainerEntity>
class that takes care of implementing theIAsyncLifetime
interface for starting and disposing the container. This reduces boilerplate code in all test projects.Why is it important?
This removes code duplication, thus reducing maintenance. Also, adding a timeout when starting a container during the tests could now be performed at a single place (
ContainerTest.cs
) instead of at 24 different places.Related issues
PR #908 is somehow related in that it simplifies the maintenance of the tests but both PRs can be merged independently.