search

colopl / laravel-spanner

Laravel database driver for Google Cloud Spanner
Apache License 2.0
97 stars 16 forks source link

dropAll() return immediately if no tables found #193

Closed matthewjumpsoffbuildings closed 8 months ago

matthewjumpsoffbuildings commented 8 months ago

Small tweak to Schema Builder dropAll() - if no tables exists simply return immediately

taka-oyama commented 8 months ago

Please add a test that runs this code.

matthewjumpsoffbuildings commented 8 months ago

Im actually not sure how to test this? Since dropAll() returns void, how would I test if it exited early due to there being no tables, or if it exited later because it found and dropped some tables?

taka-oyama commented 8 months ago

I'm ok as long as all the code paths are covered. So you can just run Connection::dropAllTables with no tables and just $this->assertTrue(true);.

matthewjumpsoffbuildings commented 8 months ago

added test_dropAllTablesEmpty(), that should cover it?

matthewjumpsoffbuildings commented 8 months ago

It appears to work when I run it, but I am not sure about a couple of things

Does that test_dropAllTablesEmpty() get run last in the file? If so it will be okay since the previous test_dropAllTables() will have emptied the db. Or do the test methods in the file get called in any order, in which case does each test method start with an empty database, or do artefacts from previous tests persist?

taka-oyama commented 8 months ago

I think tests that depend on other tests to be run first is really bad.

So ideally, there should be a hook which deletes all created tables after every test, but that slows down the entire test significantly. As a compromise, I think you should drop all tables twice in this test. Once to actually clear all tables, and 2nd run which will guarantee that the given code is run.

matthewjumpsoffbuildings commented 8 months ago

Done

taka-oyama commented 8 months ago

Please add a changelog

matthewjumpsoffbuildings commented 8 months ago

Updated the changelog, not sure if its a new point release, please edit if needed

Also should we add #191 and #192 to the changelog too?

taka-oyama commented 8 months ago

Also should we add https://github.com/colopl/laravel-spanner/pull/191 and https://github.com/colopl/laravel-spanner/pull/192 to the changelog too?

Yes but I'd like to have a different PR for them.

matthewjumpsoffbuildings commented 8 months ago

Yes but I'd like to have a different PR for them.

Do you want me to do anything or you got it?

taka-oyama commented 8 months ago

I'd appreciate it if you can write the ones for the previous two. Thanks.

matthewjumpsoffbuildings commented 8 months ago

Which branch should I do it on?

taka-oyama commented 8 months ago

master for all unreleased features.