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

dropAll() return immediately if no tables found #193

Merged
merged 9 commits into from
Mar 5, 2024

Conversation

matthewjumpsoffbuildings
Copy link
Contributor

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

@taka-oyama
Copy link
Collaborator

Please add a test that runs this code.

@matthewjumpsoffbuildings
Copy link
Contributor Author

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
Copy link
Collaborator

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);.

@taka-oyama taka-oyama added the enhancement New feature or request label Mar 4, 2024
@matthewjumpsoffbuildings
Copy link
Contributor Author

added test_dropAllTablesEmpty(), that should cover it?

@matthewjumpsoffbuildings
Copy link
Contributor Author

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
Copy link
Collaborator

taka-oyama commented Mar 5, 2024

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
Copy link
Contributor Author

Done

@taka-oyama taka-oyama self-assigned this Mar 5, 2024
@taka-oyama
Copy link
Collaborator

Please add a changelog

@matthewjumpsoffbuildings
Copy link
Contributor Author

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?

CHANGELOG.md Outdated Show resolved Hide resolved
@taka-oyama
Copy link
Collaborator

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

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

@taka-oyama taka-oyama self-requested a review March 5, 2024 05:39
@taka-oyama taka-oyama merged commit 702e92f into colopl:master Mar 5, 2024
1 check passed
@matthewjumpsoffbuildings
Copy link
Contributor Author

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
Copy link
Collaborator

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

@matthewjumpsoffbuildings
Copy link
Contributor Author

Which branch should I do it on?

@taka-oyama
Copy link
Collaborator

master for all unreleased features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants