Skip to content

Conversation

vpavic
Copy link
Contributor

@vpavic vpavic commented Mar 20, 2016

This resolves #419.

I've signed the CLA.

@vpavic
Copy link
Contributor Author

vpavic commented Mar 20, 2016

Closing & re-opening to force a new CI build.
Failure seems unrelated to these changes, the build passes for me locally.

@vpavic vpavic closed this Mar 20, 2016
@vpavic vpavic reopened this Mar 20, 2016
@rwinch
Copy link
Member

rwinch commented Mar 20, 2016

Thanks for the PR! I wonder if we want to enable this for the sample applications too? What are your thoughts @vpavic?

@rwinch rwinch self-assigned this Mar 20, 2016
@rwinch rwinch added the status: waiting-for-feedback We need additional information before we can continue label Mar 20, 2016
@vpavic
Copy link
Contributor Author

vpavic commented Mar 20, 2016

@rwinch I've considered using this in sample app too but eventually decided to limit use to integration tests.

My reasoning was the following:

  • Flapdoodle on its website clearly states the project is aimed at running Mongo in tests so we should follow this
  • IMO sample apps should provide a realistic use case of how to use the framework
  • sample apps include instructions on how to browse the session data using clients for underlying technology used to persist sessions, meaning you need Mongo (client at least) anyway to fully benefit from sample app (and I don't know how Mongo client would work with an embedded Flapdoodle instance)

This is in contrast to JDBC samples for example, which use H2 database, since H2 is full-blown RDBMS which also offers easily embeddable web client.

@rwinch
Copy link
Member

rwinch commented Mar 21, 2016

@vpavic Thank you for your feedback. I somewhat see your point about the samples needing to provide a realistic experience for users. However, we do not want any extra noise for setting up samples.

Fladoodle should certainly not be used in production, but I think it is ideal for helping users run a sample application. We are testing the sample with Fladoodle so that is how users should experience it (since we know that is what works).

Finally Fladoodle is just downloading the executable and forking a process that starts up a full fledged Mongo instance. This means that this does not differ much from using an embedded relational database.

@vpavic
Copy link
Contributor Author

vpavic commented Mar 21, 2016

@rwinch if we go with Flapdoodle to back the sample app, what happens with the part of the guide that describes usage of Mongo client to query and alter data (how does it work section)?

@rwinch
Copy link
Member

rwinch commented Mar 22, 2016

@vpavic You can still connect to mongo when using Fladoodle using any Mongo client.

@vpavic
Copy link
Contributor Author

vpavic commented Mar 22, 2016

@rwinch yes, I'm aware of that. But if you have Mongo client it's highly likely you also have Mongo server installed as well, that was one of my arguments for not moving sample app to Flapdoodle.

But OK, if that's your preference I'll move the sample app to Flapdoodle.

There's one more thing that crossed my mind today, and I've completely neglected it during initial implementation - embedded Flapdoodle instance needs to use random port to avoid any potential conflicts. That means in sample app scenario we also need to communicate this port to the user - is logging the port on startup good enough for this?

@rwinch
Copy link
Member

rwinch commented Mar 22, 2016

@vpavic Thanks for the response.

But if you have Mongo client it's highly likely you also have Mongo server installed as well, that was one of my arguments for not moving sample app to Flapdoodle.

That is fair. However, I think there are many people that will not care about performing the steps with the client (they just want to get it running). We should have a disclaimer that you must have a client installed within the guide.

That means in sample app scenario we also need to communicate this port to the user - is logging the port on startup good enough for this?

This is an option. Alternatively (and probably preferably), we could default the port to using the standard port and ensure our test configuration overrides this to a random port.

@vpavic
Copy link
Contributor Author

vpavic commented Mar 22, 2016

We should have a disclaimer that you must have a client installed within the guide.

OK.

Alternatively (and probably preferably), we could default the port to using the standard port and ensure our test configuration overrides this to a random port.

Even though this means sample app fails to start for users that run Mongo instance locally? This is somewhat in contradiction to "we do not want any extra noise for setting up samples" argument.

@rwinch
Copy link
Member

rwinch commented Mar 22, 2016

@vpavic

Even though this means sample app fails to start for users that run Mongo instance locally? This is somewhat in contradiction to "we do not want any extra noise for setting up samples" argument.

That is a good point. Let's stick with logging. Perhaps it could even log "if you have the mongo client installed this is the command to connect"?

@vpavic
Copy link
Contributor Author

vpavic commented Mar 22, 2016

@rwinch yes, that should be doable. I'll look into it and update the PR accordingly soon.

@vpavic vpavic force-pushed the improve-mongo-it branch from 65185b3 to ed08593 Compare March 22, 2016 23:21
@vpavic
Copy link
Contributor Author

vpavic commented Mar 22, 2016

@rwinch I've updated the PR, please review the changes.
I still need to update the guide. I'll take care of this when we finalize the implementation.

*/
@ContextConfiguration
@DirtiesContext
Copy link
Member

Choose a reason for hiding this comment

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

Just curious why @DirtiesContext was necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good thing you pointed this out, @DirtiesContext can be removed now.

I've had issues similar to ones described in #396 with my initial implementation, since I had static port config for embedded MongoDB instance.

Moving to randomly assigned port fixed this.

@rwinch
Copy link
Member

rwinch commented Mar 23, 2016

@vpavic Thanks for the fast turnaround. Overall looks good. I had one comment inline. When you update the documentation make sure that the guide either does not show the Flapdoodle config or it describes that this is just necessary to start mongo and not going to happen in a real world example.

@vpavic
Copy link
Contributor Author

vpavic commented Mar 24, 2016

@rwinch

When you update the documentation make sure that the guide either does not show the Flapdoodle config or it describes that this is just necessary to start mongo and not going to happen in a real world example.

Yes, of course. Mongo-related config in sample app is in a separate @Configuration class, the original @Configuration class that contains asciidoc tags was left untouched.

@vpavic
Copy link
Contributor Author

vpavic commented Mar 24, 2016

It completely slipped my mind that we could use Spring Boot's auto-configuration for an embedded MongoDB instance. I'll change this in next update.

@vpavic vpavic force-pushed the improve-mongo-it branch from ed08593 to f2017aa Compare March 24, 2016 22:05
@vpavic
Copy link
Contributor Author

vpavic commented Mar 24, 2016

@rwinch I've updated the PR with the documentation/guide.

I also tried the Boot's Embedded MongoDB auto-configuration support, however I've run into spring-projects/spring-boot#5487. Not to confuse the users with this exception (even though its harmless as it occurs on shutdown) I've decided to stick with manual config until this gets fixed in Boot. Your thoughts?


UPDATE:

Somehow I managed to miss the fact that this exception on shutdown also happens when we use manual conf. So I'll update this to auto-configuration anyway. Also it appears that nothing within Boot itself would be able to fix this behavior.

@vpavic vpavic force-pushed the improve-mongo-it branch from f2017aa to 903cac4 Compare March 25, 2016 06:28
@rwinch rwinch added status: duplicate A duplicate of another issue and removed status: waiting-for-feedback We need additional information before we can continue labels Mar 25, 2016
@rwinch rwinch added this to the 1.2.0 milestone Mar 25, 2016
@rwinch rwinch merged commit 79928bd into spring-projects:master Mar 25, 2016
@rwinch
Copy link
Member

rwinch commented Mar 25, 2016

@vpavic Thanks for digging into this so deep and for the quality PR! This is now merged into master

@rwinch rwinch self-assigned this Mar 25, 2016
@vpavic vpavic deleted the improve-mongo-it branch March 25, 2016 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: duplicate A duplicate of another issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Embedded Mongo for integration tests & samples
2 participants