Skip to content
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

Testcontainer for CosmosDB Emulator #549

Closed

Conversation

Yeseh
Copy link
Contributor

@Yeseh Yeseh commented Aug 2, 2022

I seem to be on the right track, so I'm opening a PR for some feedback.
General solution direction is to use a custom HttpClientHandler to query the database using the REST API instead of using the CosmosDB client SDK.

Here's my current view of the TODOs:

  • Setup basic framework for testcontainer
  • Implement HttpClient for use with the CosmosDB REST API
  • Create a database in the container
  • Find a better WaitStrategy (especially with high partition counts)
  • Execute an arbitrary query
  • Allow for multi-port binding in testcontainer to expose other CosmosDB endpoints (also needed for storage emulator)

Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution and pull request 🙏.

I have added some suggestions (or questions). I did not review CosmosDbHttpHandler incl. the tests. I will do that at the end. Please consider to fix the code smells (warnings) too, thanks.

@Yeseh
Copy link
Contributor Author

Yeseh commented Aug 8, 2022

Thanks for the thourough review! Will get back to this later this week hopefully.

@Yeseh
Copy link
Contributor Author

Yeseh commented Aug 28, 2022

Still WIP but improved a bit to model it after the recent work on the Azurite container. I came to the conclusion that it's kinda nuts to implement the client stuff in the container, can just let the user take care of that :).

@Yeseh Yeseh force-pushed the feature/cosmosdb-testcontainer branch from 0b183b3 to dad2dc9 Compare August 28, 2022 14:51
@ktjn
Copy link
Contributor

ktjn commented Sep 3, 2022

@Yeseh You are not connecting the output consumer. Add: .WithOutputConsumer(configuration.OutputConsumer). Also add a this.OutputConsumer = Consume.RedirectStdoutAndStderrToStream(new MemoryStream(), new MemoryStream());

If you want I can help out with this.

@Yeseh
Copy link
Contributor Author

Yeseh commented Sep 4, 2022

@Yeseh You are not connecting the output consumer. Add: .WithOutputConsumer(configuration.OutputConsumer). Also add a this.OutputConsumer = Consume.RedirectStdoutAndStderrToStream(new MemoryStream(), new MemoryStream());

If you want I can help out with this.

Ah that makes a lot of sense. Be my guest with helping, i'm massively short on time atm :)

@ktjn
Copy link
Contributor

ktjn commented Sep 5, 2022

Ok. I currently have a working CosmosDbFixture which can be used to fix this. One thing to note is that the host is not always "localhost" depending on where docker is running. I can fork your repo or you can give me access to your witchevey you prefer.

@ktjn
Copy link
Contributor

ktjn commented Sep 6, 2022

@Yeseh Did some quick changes to make the test work. I think the mongo api should be separated into some subclass. Messy with all properties.

@Yeseh
Copy link
Contributor Author

Yeseh commented Sep 6, 2022

@ktjn Thanks a lot!

Definitely agree on the mongo part. Think the best route is a separate testcontainer altogether, because of all the differences. What do you think?

@ktjn
Copy link
Contributor

ktjn commented Sep 6, 2022

@ktjn Thanks a lot!

Definitely agree on the mongo part. Think the best route is a separate testcontainer altogether, because of all the differences. What do you think?

Yes, at least do the sql mode first. Btw, I saw the build was stuck. It might have to do with "localhost" and running on linux.

I guess we should try to remove the cosmosdb client dependency and go for a pure http based test as well?

@ktjn
Copy link
Contributor

ktjn commented Sep 6, 2022

It's getting stuck at:
2022-09-06 20:51:19.295 +02:00 [INF] DotNet.Testcontainers.Tests.Initialized Attach "DotNet.Testcontainers.Configurations.RedirectStdoutAndStderrToStream" at Docker container "c9021df372bcf28a38a87946b07b6c077c3f22b0659e7ad7192a3639899a6860"
2022-09-06 20:51:19.297 +02:00 [INF] DotNet.Testcontainers.Tests.Initialized Start Docker container "c9021df372bcf28a38a87946b07b6c077c3f22b0659e7ad7192a3639899a6860"

I works fine on windows and wsl. I'll setup a linux vm and will try it there.

@ktjn
Copy link
Contributor

ktjn commented Sep 8, 2022

The test still works on a linux vm. Added a 5 min timeout so it doesn't run forever. Might need to add some logging to understand the problem.

@HofmeisterAn
Copy link
Collaborator

Do you need any help? Let me know if I should take a look at something.

@ktjn
Copy link
Contributor

ktjn commented Sep 8, 2022

Do you need any help? Let me know if I should take a look at something.

Thanks. No, just wanted to understand why the container didn't start correctly. But when adding some debug everything ran ok. I want a clean working baseline before I help @Yeseh clean this up.

@ktjn
Copy link
Contributor

ktjn commented Sep 8, 2022

@HofmeisterAn Can you please stop the build? my task.delay hack did fail the test but the wait strategy is still running? Can I set a timeout for the tests until this problem is found?

@ktjn
Copy link
Contributor

ktjn commented Sep 25, 2022

Ok, so I tried setting the ip of the host. Took me a while to figure out that the values wren't actually used(fix this now at least). Still the get accountinfo works but the database operation times out.

Any idea @JonasBenz?

@ktjn
Copy link
Contributor

ktjn commented Sep 26, 2022

Did some more investigation. The url rewriter is by far the most efficeient way to get the cosmos emulator running in docker.

@JonasBenz
Copy link
Contributor

JonasBenz commented Sep 26, 2022

Did some more investigation. The url rewriter is by far the most efficeient way to get the cosmos emulator running in docker.

So setting the IP of the docker host as environment variable AZURE_COSMOS_EMULATOR_IP_ADDRESS_OVERRIDE like @HofmeisterAn suggested here did not work?: #549 (comment)

If the UrlRewriter is the better solution, I would prefer to provide a property of type HttpMessageHandler, instead of HttpClient because of the reasons described here: #549 (comment)

Or providing both HttpMessageHandler and HttpClient would also be an option.

@ktjn
Copy link
Contributor

ktjn commented Sep 26, 2022

Or providing both HttpMessageHandler and HttpClient would also be an option.

Done.

@ktjn
Copy link
Contributor

ktjn commented Sep 26, 2022

@HofmeisterAn I guess this PR is ready for review.

@HofmeisterAn
Copy link
Collaborator

@HofmeisterAn I guess this PR is ready for review.

👏 I will do it in the next days. I hope it is not urgent.

@Yeseh Yeseh marked this pull request as ready for review September 29, 2022 15:14
Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

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

Looks good. I have just a few more small suggestions / improvements.

@HofmeisterAn
Copy link
Collaborator

07c6786, does it close #421? Or is anything major left? Does it make sense to create a new issue?

@ktjn
Copy link
Contributor

ktjn commented Oct 5, 2022

Maybe for fixing the tests when the emulator is stable.

@HofmeisterAn
Copy link
Collaborator

Maybe for fixing the tests when the emulator is stable.

Then I will create a separate issue. Other than that the Cosmos DB Emulator is done?

@ktjn
Copy link
Contributor

ktjn commented Oct 5, 2022

Then I will create a separate issue. Other than that the Cosmos DB Emulator is done?

Yes!

@HofmeisterAn
Copy link
Collaborator

07c6786, closes #421.

@Yeseh Yeseh deleted the feature/cosmosdb-testcontainer branch October 6, 2022 08:35
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.

None yet

4 participants