silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
722 stars 821 forks source link

Run automated tests with SQLite to speed up local testing #8452

Closed chillu closed 4 years ago

chillu commented 6 years ago

We've removed this via https://github.com/silverstripe/silverstripe-framework/commit/c47a1d9091c4cba52109c7fa9d9a46ac57d4ea93 with the rationale that we don't run SQLite in production, and that it delays builds in our constrained free Travis builder allocation.

There's been a recent push in our product teams to test using https://github.com/silverstripe/cwp-recipe-kitchen-sink with a full combination of modules. This makes unit tests extremely slow. There's other ways to improve that, but the quickest win is to run them via in-memory SQLite. At the moment, a dev doing local development can either choose to write code in kitchen sink, which is good enough for manual regression testing, and discovering side effects from other modules. Or they optimise for TDD and local test execution speed, in which case running the kitchen sink is less feasible.

I'm keen for devs to use a fast way of local testing, which encourages writing more tests, and also avoids expensive context switches when a full test suite fails on Travis hours after you've thought about that piece of code.

In order to achieve this, I'm recommending that we add SQLite as nightly builds to the kitchen sink: https://github.com/silverstripe/cwp-recipe-kitchen-sink/blob/master/.travis.yml. This would further use up our builder capacity in Travis, and potentially time out the entire build. @robbieaverill Do you know how long a Travis build can run in total, or if there are limitations on how many build jobs it can run?

robbieaverill commented 6 years ago

Do you know how long a Travis build can run in total, or if there are limitations on how many build jobs it can run?

I think it's about 40-45 minutes per job. We have 15 jobs on the kitchen sink, I'm not sure what the limit is there though, but you get about 5 concurrent jobs. Both statements are referring to the free version.

ScopeyNZ commented 5 years ago

If it were run single-threaded it would take between 3-4 hours. We get a report in Slack showing the single-threaded time when the build is finished.

Divide that by ~5 and you get an approximate "real" time.

michalkleiner commented 5 years ago

Just came across this issue in my local dev env.

I couldn't run local framework tests using SQLite database and it seems Travis would be broken as well.

Getting an error where Injector can't create the db class:

Uncaught SilverStripe\Core\Injector\InjectorNotFoundException: ReflectionException: Class SQLite3PDODatabase does not exist in /github/silverstripe-framework/src/Core/Injector/InjectionCreator.php:17

Followed what Travis is doing and ran composer require silverstripe/recipe-testing:^1 silverstripe/recipe-core:4.4.x-dev composer require silverstripe/sqlite3:2.0.x-dev, tried also 2.2.x-dev with no luck

Not sure if this is somehow related to https://github.com/silverstripe/silverstripe-sqlite3/issues/53 as well.

NightJar commented 5 years ago

That Injector unknown service name error shouldn't happen.

https://github.com/silverstripe/silverstripe-sqlite3/blob/2/_config/connectors.yml#L5

The linked issue sees the tests run @michalkleiner - just not from memory (as they're supposed to).

michalkleiner commented 5 years ago

Created a temp branch to check SQLite build in Travis https://travis-ci.org/silverstripe/silverstripe-framework/jobs/582996761

michalkleiner commented 5 years ago

Failed on something else though, will give a local fresh test another go.

michalkleiner commented 5 years ago

After merging in https://github.com/silverstripe/silverstripe-framework/pull/9234 the SQLite tests now run.

robbieaverill commented 4 years ago

Thanks for fixing the SQLite builds @michalkleiner.


My main issue with using SQLite in general for unit tests is that it doesn't represent a realistic environment for testing in, since people don't usually use SQLite in production sites. I'd rather focus on improving the test harness so you can easily isolate the database and run actual unit tests, in order to improve the test run time.

-1 from me, sorry!

dnsl48 commented 4 years ago

I agree with everything @robbieaverill just said.

I'm keen for devs to use a fast way of local testing, which encourages writing more tests, and also avoids expensive context switches when a full test suite fails on Travis hours after you've thought about that piece of code.

I would suggest we invest the effort into adopting paratest or something alike, rather than trying to make the rdbms faster.

mi-hol commented 4 years ago

I'm keen for devs to use a fast way of local testing, which encourages writing more tests, and also avoids expensive context switches when a full test suite fails on Travis hours after you've thought about that piece of code.

I would suggest we invest the effort into adopting paratest or something alike, rather than trying to make the rdbms faster.

Well finding the right balance for conflicting needs seems to be the topic here. To me 'make the rdbms faster' seems a quick fix, requiring no further effort and is ready for use NOW, while ' adopting paratest or something alike' seems to be the right mid- to long term goal but will require considerable effort to implement, hence have a much longer lead time before we can harvest the benefits. So there is room for a 2 prong approach.

Just my 5 cent ;-)

dnsl48 commented 4 years ago

Fair enough. On the other hand, I'm not completely sure SQLite is ready for use and requires no further effort. It's definitely available for local test runs already. However, I doubt it will be green everywhere out of the box if we start running it for all modules on CI.

ScopeyNZ commented 4 years ago

I'm a -1 on this too. Although we could probably encourage an in-memory SQLite config for local development where speed is important, when on CI, the speed isn't so much of a factor. Better for CI to more closely represent your production environment.

In terms of SQLite on module repos, I'm definitely not against trying to bring SQLite into the list of properly supported DB adapters - which is what the intent of this issue would require anyway. If we do that, then putting SQLite on a build in our travis configs makes sense.

chillu commented 4 years ago

Yeah this is a tough one. We're trying to limit rather than expand the module surface that we support, so to that end we probably shouldn't add SQLite builds to a supported recipe.

There's a few points which currently make testing harder than it should be in Silverstripe:

Unfortunately the answer for those is "refactor almost everything" (e.g. separate business objects from persistence in the ORM, DataMapper rather than ActiveRecord pattern). Which is not achievable in a reasonable timeframe for devs who want to follow test-driven development practices.

I've been thinking about an additional benefit of SQLite in terms of an easier onboarding experience: With SQlite installed (e.g. on OSX), you could get Silverstripe going through a simple php -S without any other mucking around with LAMP. But that's less relevant in a world of cloud providers and containers now as well.

So in conclusion, I don't see us increasing our current level of investment in SQLite to the extent that it would make it feasible what I originally suggested in this issue, hence closing it.