-
-
Notifications
You must be signed in to change notification settings - Fork 288
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
feat: Introduce a new Testcontainers.Xunit package #1165
feat: Introduce a new Testcontainers.Xunit package #1165
Conversation
✅ Deploy Preview for testcontainers-dotnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
680b863
to
93f372d
Compare
93f372d
to
3641802
Compare
I think we can consolidate a few things. The underlying configuration for a single instance ( |
Excellent point! It did not even come to my mind that the lifetime implementation could be shared. I have done it in c20188e with a new |
Sorry for the late response today. Tuesdays are always busy for me.
👍 We do not need to apply the suggestion, but I would like to share and discuss it quickly because I think the code (xUnit's shared context) has a lot in common, and some parts feel like copy and paste. The example I am sharing is not complete because I had to remove some critical parts, but the important parts are still there, and the concept should not be very difficult to understand. Please also take into account that the example contains much more than what is actually necessary for Testcontainers. The internal sealed class XunitLoggerProvider : ILoggerProvider
{
private readonly Stopwatch _stopwatch = Stopwatch.StartNew();
private readonly ITestOutputHelper _testOutputHelper;
public XunitLoggerProvider(IMessageSink messageSink)
{
_testOutputHelper = new MessageSinkTestOutputHelper(messageSink);
}
public XunitLoggerProvider(ITestOutputHelper testOutputHelper)
{
_testOutputHelper = testOutputHelper;
}
public void Dispose()
{
}
public ILogger CreateLogger(string categoryName)
{
return new XunitLogger(_stopwatch, _testOutputHelper, categoryName);
}
private sealed class MessageSinkTestOutputHelper : ITestOutputHelper
{
private readonly IMessageSink _messageSink;
public MessageSinkTestOutputHelper(IMessageSink messageSink)
{
_messageSink = messageSink;
}
public void WriteLine(string message)
{
_messageSink.OnMessage(new DiagnosticMessage(message));
}
public void WriteLine(string format, params object[] args)
{
_messageSink.OnMessage(new DiagnosticMessage(format, args));
}
}
private sealed class XunitLogger : ILogger
{
private readonly Stopwatch _stopwatch;
private readonly ITestOutputHelper _testOutputHelper;
private readonly string _categoryName;
public XunitLogger(Stopwatch stopwatch, ITestOutputHelper testOutputHelper, string categoryName)
{
_stopwatch = stopwatch;
_testOutputHelper = testOutputHelper;
_categoryName = categoryName;
}
public IDisposable BeginScope<TState>(TState state)
{
return Disposable.Instance;
}
public bool IsEnabled(LogLevel logLevel)
{
return logLevel != LogLevel.None;
}
public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func<TState, Exception?, string> formatter)
{
_testOutputHelper.WriteLine("[{0} {1:hh\\:mm\\:ss\\.ff}] {2}", _categoryName, _stopwatch.Elapsed, formatter.Invoke(state, exception));
}
private sealed class Disposable : IDisposable
{
private Disposable()
{
}
public static IDisposable Instance { get; } = new Disposable();
public void Dispose()
{
}
}
}
} I had to remove some parts here that I cannot share. The example builds on top of the public abstract class CustomWebApplicationFactory<TEntryPoint> : WebApplicationFactory<TEntryPoint>, IAsyncLifetime where TEntryPoint : class
{
private readonly List<IWebHostConfiguration> _configurations = new();
protected CustomWebApplicationFactory()
{
AddWebHostConfiguration(new ClearLoggingProviders());
}
public void AddWebHostConfiguration(IWebHostConfiguration configuration)
{
_configurations.Add(configuration);
}
protected override void ConfigureWebHost(IWebHostBuilder builder)
{
base.ConfigureWebHost(builder);
foreach (var configuration in _configurations)
{
configuration.ConfigureWebHost(builder);
}
}
// For Testcontainers, the AsyncLifetime property is not necessary; this is specific to the type clash (WebApplicationFactory<TEntryPoint>).
public IAsyncLifetime AsyncLifetime => this;
public ValueTask InitializeAsync()
{
// Due to a naming conflict where the interfaces `IAsyncLifetime` and
// `IAsyncDisposable` share the same name for the member `DisposeAsync`, but have
// different return types, we have implemented the members of the `IAsyncLifetime`
// interface explicitly. To prevent warnings, a non-explicit implementation has
// been added. This matter will be addressed in xUnit.net version 3, which also
// utilizes the `IAsyncDisposable` interface.
throw new InvalidOperationException("Use the delegate property AsyncLifetime instead.");
}
async Task IAsyncLifetime.InitializeAsync()
{
await Task.WhenAll(_configurations.Select(configuration => configuration.InitializeAsync()))
.ConfigureAwait(false);
}
async Task IAsyncLifetime.DisposeAsync()
{
await Task.WhenAll(_configurations.Select(configuration => configuration.DisposeAsync()))
.ConfigureAwait(false);
}
public class SharedInstance : CustomWebApplicationFactory<TEntryPoint>
{
public SharedInstance(IMessageSink messageSink)
{
AddWebHostConfiguration(new LoggingWebHostConfiguration(messageSink));
}
}
public class SingleInstance : CustomWebApplicationFactory<TEntryPoint>
{
private readonly MoqLoggerProvider _mockOfILogger = new();
public SingleInstance(ITestOutputHelper testOutputHelper)
{
AddWebHostConfiguration(new LoggingWebHostConfiguration(testOutputHelper));
AddWebHostConfiguration(new LoggingWebHostConfiguration(_mockOfILogger));
}
public Mock<ILogger> MockOfILogger => _mockOfILogger;
protected override void Dispose(bool disposing)
{
base.Dispose(disposing);
_mockOfILogger.Dispose();
}
}
private sealed class ClearLoggingProviders : WebHostConfiguration
{
public override void ConfigureWebHost(IWebHostBuilder builder)
{
builder.ConfigureTestServices(services => services.RemoveAll<ILoggerFactory>());
builder.ConfigureLogging(loggingBuilder => loggingBuilder.ClearProviders());
}
}
} The test class will use either one of the instances, similar to: public class XunitShardContext1 : IClassFixture<CustomWebApplicationFactory<Program>.SharedInstance>
{
private readonly CustomWebApplicationFactory<Program> _app;
public XunitShardContext1(CustomWebApplicationFactory<Program>.SharedInstance app)
{
_app = app;
}
}
public class XunitShardContext2 : IAsyncLifetime
{
private readonly CustomWebApplicationFactory<Program> _app;
public XunitShardContext2(ITestOutputHelper testOutputHelper)
{
_app = new CustomWebApplicationFactory<Program>.SingleInstance(testOutputHelper);
}
} |
c20188e
to
04f10ae
Compare
@0xced, are there any tasks left? It would be great to finalize the PR and merge it. I think many developers can benefit from this package. Please, no hurry — I just want to double-check if there are any tasks left. I can take care of them too if you need help. |
I’m pretty happy with the current implementation. I have another branch ( Do you plan to release a preview package first so that we could gather some feedback before committing to a stable API ? Finally, except for the xmldoc, no documentation has been written. I can start writing documentation next week. |
042faa3
to
0d7bf77
Compare
I will try to review the PR sometime this week. Afterward, I can publish a snapshot version 👍. |
f2951b1
to
cf7c8d2
Compare
bcf0141
to
2639868
Compare
* Use a NullScope object * Check for logLevel != LogLevel.None
Uses the same source files as the Testcontainers.Xunit project (with some conditional compilation) and targeting [xUnit.net v3](https://xunit.net/docs/getting-started/v3/migration).
Caused by xunit/xunit#3001 but it turns out to be more elegant not to have to inject `IMessageSink` in the fixtures.
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.
Thanks for your awesome work. I took the time to slightly update the documentation and added a test project to ensure that the basic features work as expected, LMKWYT. Additionally, I made a small change to how we instantiate the logger instances.
I have two small follow-up questions. I think the PR looks good, and we can merge it after we address them. WDYT?
/// <typeparam name="TBuilderEntity">The builder entity.</typeparam> | ||
/// <typeparam name="TContainerEntity">The container entity.</typeparam> | ||
[PublicAPI] | ||
public abstract class ContainerTest<TBuilderEntity, TContainerEntity>(ITestOutputHelper testOutputHelper, Func<TBuilderEntity, TBuilderEntity> configure = null) |
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.
Why do we provide the configure
overload here? Wouldn't it be sufficient to expect developers to override the member?
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 initially wrote it like that for no particular reason. We can probably get rid of it, if only for consistency with how it's done with ContainerFixture
.
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.
Wait, I take that back, it's pretty convenient actually. For example:
public abstract class MongoDbContainerTest(ITestOutputHelper testOutputHelper, Func<MongoDbBuilder, MongoDbBuilder> configure = null)
: ContainerTest<MongoDbBuilder, MongoDbContainer>(testOutputHelper, configure)
{
[Fact]
public void Test()
{
}
[UsedImplicitly]
public sealed class MongoDbDefaultConfiguration(ITestOutputHelper testOutputHelper)
: MongoDbContainerTest(testOutputHelper);
[UsedImplicitly]
public sealed class MongoDbNoAuthConfiguration(ITestOutputHelper testOutputHelper)
: MongoDbContainerTest(testOutputHelper, builder => builder.WithUsername(string.Empty).WithPassword(string.Empty));
[UsedImplicitly]
public sealed class MongoDbV5Configuration(ITestOutputHelper testOutputHelper)
: MongoDbContainerTest(testOutputHelper, builder => builder.WithImage("mongo:5.0"));
[UsedImplicitly]
public sealed class MongoDbV4Configuration(ITestOutputHelper testOutputHelper)
: MongoDbContainerTest(testOutputHelper, builder => builder.WithImage("mongo:4.4"));
}
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.
This is probably an interesting use case, indeed. We should document it somehow, otherwise no one will know about the overload.
Either this or release a preview version of the
Awesome work on the documentation! I'd just change the
Overall this looks good except for this specific change (commit 630a073). I think it's worse than the previous implementation in many ways.
|
Yes, I was thinking about that too, but I would like to reuse existing pulled images and avoid pulling more versions, since we had issues in the past with running out of disk space (I know, the Redis image is tiny). Maybe we can use a delegate that contains the version number in its name to make it more comprehensive. ✅ 2e79dd7: I replaced the const with a plain string and assert we are using the same version.
I don't think so. The previous one was inconsistent in its log messages (this was the main reason I changed it). This is probably a matter of taste, but simply using fewer lines is not a valid argument. If necessary, you can reduce the
This is not an issue, as the implementation is not public. We can change this behavior in the future when Xunit.net V3 is officially available. Until then, a lot will probably change anyway.
Yes, this is something I wanted to ask you about. It's good that you brought it up. My idea was to support a default constructor that doesn't expect any arguments and then uses a
Probably, we are expecting different behavior here. I intended to log it once per fixture, but it looks like you want to log it once for the entire ✅ 2e79dd7: I overrode the Edit: Something I wanted to mention is that in the future, I would like to transition to using a simple annotation that we can add to an |
This inconsistency was deliberate because a timestamp is already included by xUnit for diagnostic messages. Previous version (without additional timestamps in the
After "fix" for consistency:
I think we don't want this double timestamps with a few milliseconds of difference.
It's not really the number of lines but more the cognitive overhead implementing the
I still think that the initial implementation was much simpler to understand and follow. 🤷♂️ But I don't want to hold this pull request over implementation details.
It's not an issue until some test runner decides to access the
Something like this (and similar for the public class ContainerFixture<TBuilderEntity, TContainerEntity>
: ContainerLifetime<TBuilderEntity, TContainerEntity>
where TBuilderEntity : IContainerBuilder<TBuilderEntity, TContainerEntity>, new()
where TContainerEntity : IContainer
{
public ContainerFixture() : base(NullLogger.Instance)
{
}
public ContainerFixture(IMessageSink messageSink) : base(new XunitLoggerProvider(messageSink).CreateLogger("testcontainers.org"))
{
}
}
Good, because ultimately the sink writes to the same file/console output so this way we only have the runtime info logged once. Also, I saw that you removed the empty line in 313b01e but there's still one empty line if there's no label at all.
I don't understand what you mean, sorry. 🙁 Maybe it would be easier for me to get it with an example? |
Me neither; I found the previous version more difficult to comprehend. If you’d like, we can revert it. In general, I favor consistency. Regarding the
Yes 👍.
Oh, I didn’t notice that. The latest Docker Desktop update always contains the label, I will double-check. Thanks.
My intention is to support the same or similar behavior as Java does. All you need here is the
|
It seems that the recent GitHub runners update is causing issues with the tests (actions/runner-images#10649). The MSSQL container crashes on the latest ( |
…sink Test output helper: include timestamp Message sink: don't include timestamp since it's already included by xUnit
This makes Testcontainers logs optional in a better way than passing null for the IMessageSink or ITestOutputHelper.
It must be applied with the [TestCaseOrderer] attribute, not by implementing the ITestCaseOrderer interface on the test class. Changing OrderBy to OrderByDescending should make the Example2 test fail.
I did some progress.
I'm still maintaining my Testcontainers.Xunit+samples branch on top of this one where I'm using Initially I wrote this:
Now I think it should be part of a subsequent pull request. This one is big enough I think! |
This reverts commit 80b1f1d. A single constructor with an optional IMessageSink or ITestOutputHelper was a better design because it forces the consumer to opt-out of logging by explicitly passing null vs missing the constructor that takes an IMessageSink or ITestOutputHelper because a parameterless constructor exists.
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.
Thanks 🙏 Should I publish a snapshot version 🚀?
Thanks for merging.
That would be great! |
👍 It's included in |
What does this PR do?
This pull request introduces a new
Testcontainers.Xunit
NuGet package.It provides two main classes to simplify working with Testcontainers within xUnit.net tests.
ContainerTest
is a base class for tests needing one container per test method.ContainerFixture
is a fixture that can be injected into test classes that need one container per test class (a.k.a. singleton container instance).Both support logging, respectively through
ITestOutputHelper
andIMessageSink
which are the standard xUnit.net logging mechanisms.DbContainerTest
andDbContainerFixture
are also provided to ease working with database containers. They provide methods to simplify the creation ofDbConnection
instances.Why is it important?
This will greatly reduce boilerplate code needed when using Testcontainers with xUnit.net, both for Testcontainers consumers and for the Testcontainers tests themselves.
Related issues
Follow-ups
I have another branch (feature/Testcontainers.Xunit+samples) where I have started using
Testcontainers.Xunit
in the Testcontainers tests themselves. I have currently updated MongoDbContainerTest, ClickHouseContainerTest, MariaDbContainerTest and PostgreSqlContainerTest. You can have a look at them to see how the tests are simplified by using the new base classes or fixtures.I'm not sure yet whether updating all the Testcontainers tests should be part of this pull request or part of a subsequent pull request.
I've been using xUnit.net extensively so I'm pretty confident about the design of this new package. I'm not familiar enough with either NUnit or MSTest to propose similar packages, maybe someone else can step in.