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

Add MongoDB module #1961

Merged
merged 42 commits into from May 8, 2020
Merged

Add MongoDB module #1961

merged 42 commits into from May 8, 2020

Conversation

silaev
Copy link
Contributor

@silaev silaev commented Oct 8, 2019

Java8 MongoDbContainer for constructing a single node MongoDB replica set.

.travis.yml Outdated Show resolved Hide resolved
Copy link
Member

@rnorth rnorth left a comment

Choose a reason for hiding this comment

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

I'm afraid I'm still only about half way through but have run out of time for now - I think there's a few things to discuss. My last comment is possibly the trickiest thing so far.

Thank you for this work so far - hoping we can get this done!

.travis.yml Outdated Show resolved Hide resolved
docs/modules/databases/mongodb.md Outdated Show resolved Hide resolved
docs/modules/databases/mongodb.md Outdated Show resolved Hide resolved
docs/modules/databases/mongodb.md Outdated Show resolved Hide resolved
docs/modules/databases/mongodb.md Outdated Show resolved Hide resolved
@silaev
Copy link
Contributor Author

silaev commented Oct 14, 2019

Hi @rnorth,
I resolved all the discussions. Please, review them.

@silaev silaev requested a review from rnorth October 14, 2019 09:13
@rnorth rnorth self-assigned this Nov 1, 2019
@rnorth
Copy link
Member

rnorth commented Nov 1, 2019

Sorry, we prefer it if we get the chance to press the resolve button for comments, as it means we can keep track of what we've re-reviewed.

I'm going to unresolve all, but please don't take this as meaning I'm not happy with the change, it's just that I've not re-reviewed yet.

Sorry for the long time between the last review and now.

@silaev
Copy link
Contributor Author

silaev commented Nov 1, 2019

Sorry, we prefer it if we get the chance to press the resolve button for comments, as it means we can keep track of what we've re-reviewed.

I'm going to unresolve all, but please don't take this as meaning I'm not happy with the change, it's just that I've not re-reviewed yet.

Sorry for the long time between the last review and now.

Hi @rnorth ,

It's fair enough to unresolve all the discussions.
Look forward to getting a feedback from you

@rnorth
Copy link
Member

rnorth commented Feb 8, 2020

Looking at this again, I'm afraid I feel that we should simplify the testing a lot. There is a lot of code here - which means a lot of code that we have to be willing to maintain.

It seems to me that TransactionITTest is the most important test class, as it (in a black-box manner) actually verifies behaviour of the module rather than arrangement of internals. The behaviour that it verifies ('can we execute a transactional operation using the Mongo drivers') would appear to be key behaviour that matters to users, and thus the one that is most important to test for.

I'd be happy to make these changes myself if you prefer, as I'm keen that we actually get this PR integrated - sorry that it's been so long.

@silaev
Copy link
Contributor Author

silaev commented Feb 9, 2020

Looking at this again, I'm afraid I feel that we should simplify the testing a lot. There is a lot of code here - which means a lot of code that we have to be willing to maintain.

The testing code here stands for the 90%+ code coverage of MongoDB module, and yet I understand your concern about maintainability.

It seems to me that TransactionITTest is the most important test class, as it (in a black-box manner) actually verifies behaviour of the module rather than arrangement of internals. The behaviour that it verifies ('can we execute a transactional operation using the Mongo drivers') would appear to be key behaviour that matters to users, and thus the one that is most important to test for.

Indeed, TransactionITTest mostly copies the code that MongoDB offical docs set as an example

I'd be happy to make these changes myself if you prefer, as I'm keen that we actually get this PR integrated - sorry that it's been so long.

Ok, please, do it. Also, let me know if you need my help.

@rnorth
Copy link
Member

rnorth commented Feb 10, 2020

Thanks for being understanding @silaev 🙇‍♂️. I've made tweaks locally; would you mind if I push to your remote? This PR comes from your master branch, so I wanted to check before pushing.

@silaev
Copy link
Contributor Author

silaev commented Feb 10, 2020

Thanks for being understanding @silaev 🙇‍♂️. I've made tweaks locally; would you mind if I push to your remote? This PR comes from your master branch, so I wanted to check before pushing.

No, I don't mind at all, go ahead, link

@silaev silaev requested a review from bsideup April 24, 2020 07:36
@silaev
Copy link
Contributor Author

silaev commented Apr 24, 2020

@rnorth @bsideup I reduced tests to only one transaction test, also addressed all the discussions above. Could we merge this PR?

@bsideup
Copy link
Member

bsideup commented Apr 24, 2020

The docs' look weird:
https://deploy-preview-1961--testcontainers.netlify.app/modules/databases/mongodb/

I guess "Java8 MongoDbContainer for constructing a single node MongoDB replica set." is not needed as it is obvious, and the multi-node note should become a note section

Also, I just noticed MongoDb everywhere, while it is MongoDB (upper cased B) - please adjust.

@silaev
Copy link
Contributor Author

silaev commented Apr 24, 2020

@bsideup I’ve addressed all the discussions, please, check

@silaev silaev requested a review from bsideup April 24, 2020 16:57
Copy link
Member

@rnorth rnorth 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 To Me 😄 🎉

Thank you @silaev for your contribution, epic patience and flexibility as we worked through the reviews. 🙇‍♂️

@rnorth rnorth added this to the next milestone May 8, 2020
@rnorth rnorth merged commit 8377288 into testcontainers:master May 8, 2020
@silaev
Copy link
Contributor Author

silaev commented May 8, 2020

Looks Good To Me 😄 🎉

Thank you @silaev for your contribution, epic patience and flexibility as we worked through the reviews. 🙇‍♂️

Thank you, my pleasure

quincy pushed a commit to quincy/testcontainers-java that referenced this pull request May 28, 2020
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

5 participants