testcontainers / testcontainers-dotnet

A library to support tests with throwaway instances of Docker containers for all compatible .NET Standard versions.
https://dotnet.testcontainers.org
MIT License
3.65k stars 250 forks source link

feat: Embed symbols and enable continuous integration builds (deterministic source paths) #1129

Closed tom-englert closed 4 months ago

tom-englert commented 4 months ago

What does this PR do?

Use embedded symbols instead of separate symbol package

Why is it important?

Works better across all IDEs

Related issues

netlify[bot] commented 4 months ago

Deploy Preview for testcontainers-dotnet ready!

Name Link
Latest commit e6f2fea4c694ad8d2ee73970ef3aef07a513f94a
Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/65dc366f5a1c5f00080aa5e8
Deploy Preview https://deploy-preview-1129--testcontainers-dotnet.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

tom-englert commented 4 months ago

It should look like this:

I'll have a look whats wrong with this...

HofmeisterAn commented 4 months ago

It should look like this:

I see, thanks! Maybe we just need to move the properties (args) to the Cake build task (the pack task does not rebuild the projects):

diff --git a/build.cake b/build.cake
index 1d2e4ae..0843d4e 100644
--- a/build.cake
+++ b/build.cake
@@ -67,6 +67,8 @@ Task("Build")
     Verbosity = param.Verbosity,
     NoRestore = true,
     ArgumentCustomization = args => args
+      .Append("/p:ContinuousIntegrationBuild=true")
+      .Append("/p:EmbedUntrackedSources=true")
   });
 });

@@ -134,8 +136,6 @@ Task("Create-NuGet-Packages")
     SymbolPackageFormat = "snupkg",
     OutputDirectory = param.Paths.Directories.NuGetDirectoryPath,
     ArgumentCustomization = args => args
-      .Append("/p:ContinuousIntegrationBuild=true")
-      .Append("/p:EmbedUntrackedSources=true")
       .Append($"/p:Version={param.Version}")
   });
 });
tom-englert commented 4 months ago

Probably yes, and the symbols should be embedded rather than having a separate symbol package, from my experience the latter never works without friction.

tom-englert commented 4 months ago

Also maybe .Append("/p:ContinuousIntegrationBuild=true") should not really be needed, SourceLink should determine this automatically

tom-englert commented 4 months ago

https://github.com/dotnet/sourcelink/blob/a22426a46745b8fe77b9d306fa4bc1d03eff44ea/src/Microsoft.Build.StandardCI/build/Microsoft.Build.StandardCI.props#L15-L19

tom-englert commented 4 months ago

This is what I got with the current version of TestContainers:

HofmeisterAn commented 4 months ago

I published the pipeline artifacts (NuGets) to ensure that the NuGets are built and configured properly, and everything is set up correctly for the next release. It appears that ContinuousIntegrationBuild is somehow not automatically applied to the build. We need to set it explicitly (before and after).

image

tom-englert commented 4 months ago

Strange, I never had to set this - however I never used Cake, maybe that's special about this.

HofmeisterAn commented 4 months ago

Probably, we cannot enable ContinuousIntegrationBuild that easily 😐. It obviously breaks our CommonDirectoryPathTest tests. Not sure how (or if) we can solve this, because we are using the CallerFilePath attribute to resolve common directory paths:

https://github.com/testcontainers/testcontainers-dotnet/blob/5c677d9ac043d414ecc3c165c265700c98668808/src/Testcontainers/Builders/CommonDirectoryPath.cs#L61-L65

tom-englert commented 4 months ago

It obviously breaks our CommonDirectoryPathTest tests

That's strange, how can this have such a side effect? And why only on linux?

HofmeisterAn commented 4 months ago

It obviously breaks our CommonDirectoryPathTest tests

That's strange, how can this have such a side effect? And why only on linux?

I think I found the "reason" and have a potential working configuration, which also explains why you never had to set ContinuousIntegrationBuild explicitly. I need a few more minutes to validate my changes.

HofmeisterAn commented 4 months ago

If we set ContinuousIntegrationBuild to true during the build, it will break the tests because CallerFilePath now returns a deterministic root path. We can simply disable ContinuousIntegrationBuild for the test projects.

My assumption was, if we allow Cake to rebuild the packages on Pack (if the packages are already built, we likely need to force a rebuild), the ContinuousIntegrationBuild is applied automatically, and it won't interfere with the tests. Unfortunately, the configuration is not applied automatically...

HofmeisterAn commented 4 months ago

I do not think we need that many changes. The following changes work fine (incl. Sonar). We just need to make sure the pack target rebuilds the NuGets. I am still wondering why we need to set ContinuousIntegrationBuild explicitly, but maybe Source Link behaves differently if the source control provider is added explicitly.

image

tom-englert commented 4 months ago

Just needed to turn off deterministic for the tests, they don't get deployed anyhow.

tom-englert commented 4 months ago

Happy to contribute to such a great tool! Thanks for all YOUR efforts 😄