Test Performance Optimization #1463
Conversation
| @@ -10,6 +12,9 @@ internal class MartenBuild | |||
| { | |||
| private const string BUILD_VERSION = "3.11.0"; | |||
|
|
|||
| private const string DockerConnectionString = | |||
There was a problem hiding this comment.
CONCERN: That's minor, but wouldn't it be better to use the same config as in the pipeline tests? https://github.com/JasperFx/marten/pull/1463/files#diff-fec826feae04e51c0d94076385408bdcR36 It might be also good to use different database than default postgres as it might interfere with other user configuration.
| UseDefaultSchema(); | ||
| _store.Advanced.Clean.CompletelyRemoveAll(); | ||
|
|
||
| #if NET461 |
There was a problem hiding this comment.
CONCERN: Why is it needed?
|
|
||
| public void Dispose() | ||
| { | ||
|
|
There was a problem hiding this comment.
FIX: Redundant empty line.
| @@ -78,9 +89,42 @@ private static void Main(string[] args) | |||
| Target("pack", DependsOn("compile"), ForEach("./src/Marten", "./src/Marten.CommandLine", "./src/Marten.NodaTime"), project => | |||
| Run("dotnet", $"pack {project} -o ./../../artifacts --configuration Release")); | |||
|
|
|||
| Target("init-db", () => | |||
There was a problem hiding this comment.
Nice, I like that idea 👍
There was a problem hiding this comment.
I use that for something similar at work, but w/ Sql Server
| } | ||
|
|
||
| [Collection("integration")] | ||
| public class IntegrationContext : StoreContext<DefaultStoreFixture> |
There was a problem hiding this comment.
CONCERN: Why do we need few IntegrationContexts? Can't we reuse them? If they have different purposes then it would be good if they have different naming.
There was a problem hiding this comment.
There's a different integration context in the Schema project and yet another in the Noda, but they behave differently than the one, single context here
There was a problem hiding this comment.
Can we rename them to eg. HarnessIntegrationContext etc.?
|
|
||
| namespace Marten.NodaTime.Testing | ||
| { | ||
| public abstract class IntegrationContext : IDisposable |
There was a problem hiding this comment.
CONCERN: Cannot we reuse the IntegrationContext class from the main project? Maybe we could create a separate base project with base classes to reuse them between different test projects?
There was a problem hiding this comment.
It wasn't playing nicely here, and I was lazy. I don't see it doing any harm
|
@jeremydmiller it would be also good to made the base classes abstract and their constructor protected ;) |
|
I think the base classes are all protected, and I'm saying no to making the constructor protected. That's super annoying 'cause R# tries to make the subclass constructors protected. Can we just get this thing in if the tests pass? |
DocumentStoreover most of the integration tests withIntegrationContextI'm a little disappointed by the progress here, but I see a little better than a 25% reduction in execution time for the unit tests.