Skip to content

Use TestContainers for AWS Local Stack#19157

Open
hishamco wants to merge 6 commits intomainfrom
hishamco/aws-local-stack
Open

Use TestContainers for AWS Local Stack#19157
hishamco wants to merge 6 commits intomainfrom
hishamco/aws-local-stack

Conversation

@hishamco
Copy link
Copy Markdown
Member

No description provided.

@hishamco hishamco requested a review from Skrypt April 15, 2026 22:37
@hishamco
Copy link
Copy Markdown
Member Author

@Skrypt, I think this is ready to merge. Regarding your PR #19014, you could use TestContainers instead if you have some time, or I will do it once it's done from your side

@hishamco
Copy link
Copy Markdown
Member Author

Next, I will address the functional tests

@@ -1,67 +0,0 @@
name: Integration Tests
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need to be able to trigger this manually from CI

Copy link
Copy Markdown
Member Author

@hishamco hishamco Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, but we don't want all the steps

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we still should only run these tests only on PR approve automatically, not for every workflow. It's wasteful, since commits should basically never break these tests, so enough to run them just before PR merge.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No action needed here if there's an issue with using TestContainers as I understand from the comments underneath

@Skrypt
Copy link
Copy Markdown
Contributor

Skrypt commented Apr 16, 2026

For the other PR you can always create a PR that is branched on top of my PR branch.

@hishamco
Copy link
Copy Markdown
Member Author

@hishamco
Copy link
Copy Markdown
Member Author

It works fine locally if you have Docker

@hishamco hishamco requested a review from Skrypt April 17, 2026 11:02
Comment thread Directory.Packages.props
<PackageVersion Include="SixLabors.ImageSharp.Web.Providers.AWS" Version="3.2.0" />
<PackageVersion Include="StackExchange.Redis" Version="2.12.8" />
<PackageVersion Include="StyleCop.Analyzers" Version="1.1.118" />
<PackageVersion Include="Testcontainers.LocalStack" Version="4.11.0" />
Copy link
Copy Markdown
Contributor

@Skrypt Skrypt Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks good although @Piedone really wanted that we use S3Mock because it is lighter in size and executes faster. There is a Java version but no C# one on Testcontainers. Also, let's keep the integration_tests.yml file and nothing else, I don't think @Piedone will like it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the build trigger, I didn't notice any delay

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

That's 1min 55 seconds execution time added on every PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. This pulls in everything from AWS. We just need S3.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will check I remembered there's an option to specify the service, such as S3 for instance, but I need to make sure

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do that if and when we'll do that. No need to waste time until that uncertain future.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR based on the meeting feedback, but you or Jasmin can discuss that as a topic in the upcoming meeting

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this stage, this PR makes things worse in two ways, what Jasmin and I pointed out here. There's no need to discuss this at the meeting, especially not among the two of us, since we're in agreement. It's you who needs to reflect on the feedback.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't get me wrong. I understand your feedback very well :) But it would be nice if @Skrypt mentioned the concern in the last meeting - to save the time -

Based on the discussion, @sebastienros suggested using TestContainers, which is why I submitted the PR, but not forcing acceptance of it

Copy link
Copy Markdown
Contributor

@Skrypt Skrypt Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not raise a concern not knowing which Docker image is available from testcontainer. I asked questions in that meeting and I said I would try ; which is what you did here. Now, if that makes things worst now we know. Can't know without trying. Thanks for doing the work and saving me time. Now we know that we should'nt assume that testcontainer is better all the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants