search

pat / combustion

Simple, elegant testing for Rails Engines
MIT License
708 stars 51 forks source link

Avoid resetting databases for all environments, use Rails.env instead. #83

Closed wintersolutions closed 6 years ago

wintersolutions commented 6 years ago

As discussed here are my changes that make it possible to run two engine (app) instances in parallel, ie. specs and a server.

In relation to https://github.com/pat/combustion/issues/82

Hope it can be merged as is, if not I can make changes accordingly.

pat commented 6 years ago

I'm going to shift conversation about this from #82 over to here, so it's a bit more closely tied with any further code changes.

I want to summarise where this is all at to help clarify the situation. Currently: all databases listed in the internal Combustion app's database.yml are reset. With this PR: only non-standard databases plus the current database will be reset. Whichever of development, test and production aren't being used are left untouched.

The current behaviour was added by @artofhuman in #77 as part of allowing multiple databases and potentially running tests against each of them within a single test suite run - allowing different database adapters (PostgreSQL, MySQL, SQLite, etc) within a single context. I'm not sure how common this approach is (in my own gems that use Combustion, I instead use an environment variable to determine the database adapter - thus, running the suite for each adapter separately) - it feels like an edge case?

@wintersolutions from the points you've made in #82, I understand that you use the development environment for your internal Combustion app, as well as the test environment as per the standard approach. Using the development environment also feels like an edge case - this is not to say either of you are doing anything wrong, it's just that neither approach is (to my knowledge) how Combustion is regularly used.

So, where does this all leave us?

If I merge this PR, then we get a setup that works for @wintersolutions' approach, and maintains @artofhuman's as well (provided only non-standard environment names are used for additional adapters).

As for alternatives? I guess I'd lean towards a single database reset per run, rather than presuming all databases should be reset and should be required for a test suite… but that's removing existing functionality, and I'm not thrilled about that. It also breaks things for @artofhuman, which isn't nice.

I haven't thought of any other nice ways to handle this. So it may be that merging this and acknowledging the limitations of the approach is the best way to go… yes, it may not work if someone wants to use other environments (e.g. staging) treated as separate to test, but I feel the odds of such a situation are pretty miniscule.

… so, I think I've talked myself through understanding this PR, the broader context, and being happy to merge it. @artofhuman: would appreciate your input if you have the time, and @wintersolutions if any of this doesn't sound quite right, do let me know!

wintersolutions commented 6 years ago

You expressed exactly my side of the argument about this PR. I'd be glad if @artofhuman is fine with the changes too and I'll welcome the merge if possible.

pat commented 6 years ago

Going to keep the ball rolling and merge this in. @artofhuman: if you have thoughts at any point, feel free to chime in :)

pat commented 6 years ago

Just released v0.9.0 which includes this PR :)

wintersolutions commented 6 years ago

@pat thanks a lot for taking the time and releasing it!

jrochkind commented 6 years ago

yep, this is helpful to me too. I like to keep a console open in which I can play with stuff, including the stuff I've defined in the spec/internal, and keeping it open in a 'development' environment is good so I can run tests without it interfering. Glad to see it was already solved just before I needed it!