Deal with existing database in test init script by tedsecretsource · Pull Request #249 · wp-cli/scaffold-command

tedsecretsource · 2020-01-24T15:01:01Z

Fixed in issue in which the init script would fail if the database was already created. See #228 for details. I'm more than happy to write tests or whatever else is needed to get this in.

Fixed in issue in which the init script would fail if the database was already created.

nylen · 2020-01-27T02:46:20Z

 	fi

 	# create database
+	mysqladmin drop $DB_NAME -f --user="$DB_USER" --password="$DB_PASS"$EXTRA > /dev/null 2>&1


I don't think this is the right solution! Dropping the database with no warning is going to cause problems for someone eventually.

I had the same thought but isn't this script intended to be used to initialize the environment? I'm more than happy to make changes, including asking the user to confirm the deletion of the database prior to deleting it. Just let me know what seems reasonable.

asking the user to confirm the deletion of the database prior to deleting it

This seems reasonable to me but the best person to confirm is @schlessera

Added a condition to test for the existance of an existing DB, and then asking for permission before deleting it.

schlessera

You'll also need to adapt the tests in https://github.com/wp-cli/scaffold-command/blob/master/features/install-wp-tests.feature

Make the confirmation on whether or not to drop the existing database test for fewer possibilities (just y, Y, yes, Yes, and mixed case equivalents).

nylen · 2020-01-30T14:30:07Z

Just my opinion - you will want to get confirmation from @schlessera first:

This is heading in the right direction but it needs a few more fixes in order to work properly! It is also worth adding tests for the most common cases (db already exists, db doesn't exist)...

nylen · 2020-01-30T14:31:53Z


 	# create database
-	mysqladmin create $DB_NAME --user="$DB_USER" --password="$DB_PASS"$EXTRA
+	if [ $(mysql -e 'show databases;' | grep ^$DB_NAME$) ]


This command needs to use the --user="$DB_USER" --password="$DB_PASS"$EXTRA arguments for login.

Also, it is possible that the given MySQL user would have access to use the requested database, but not to list all databases. What should be done in this case?

Good catch! Fixing the missing creds now.

nylen · 2020-01-30T14:34:00Z

+		shopt -s nocasematch
+		if [[ $DELETE_EXISTING_DB =~ ^(y|yes)$ ]]
+		then
+			mysqladmin drop $DB_NAME -f --user="$DB_USER" --password="$DB_PASS"$EXTRA > /dev/null 2>&1


Why is this output being redirected to /dev/null? Shouldn't we show it instead, in case something goes wrong?

In this context yes, we should show it. Changing that now too.

tedsecretsource · 2020-01-30T21:18:17Z

Just my opinion - you will want to get confirmation from @schlessera first:

This is heading in the right direction but it needs a few more fixes in order to work properly! It is also worth adding tests for the most common cases (db already exists, db doesn't exist)...

Thanks for the feedback on what to test. I'm reading through the tests and learning how to run them now so it could be hours or days before I get another commit in incorporating your feedback but I really, really, really appreciate the feedback!

Added the missing credentials in the mysql call. Also changed the prompt in the drop db confirm to be something people are more used to seeing. Added an else case to handle instances in which the database doesn’t exists.

tedsecretsource · 2020-01-30T22:10:13Z

I'm still working on the tests but there are two things that I need input on.

I've ended up duplicating the mysqladmin create db line (wrote some WET code). How bad is this in your view?
I could probably solve this whole problem by adding set +e just before the mysqladmin create line and set -e just after it in the original version of the file to disable the script exiting on non-zero return statuses.

I think 2 is all that's needed because if the database already exists, that's ok. It's actually what we ultimately want (to have a database). It's even ok if MySQL returns an error when it tries to create it and fails because it already exists. We just don't want to stop execution of the rest of the script when that error occurs. Have I understood correctly the aim of this script and if so, does this make sense?

You'll also need to adapt the tests in https://github.com/wp-cli/scaffold-command/blob/master/features/install-wp-tests.feature

It seems this file is referenced in install-wp-tests.feature, scaffold-plugin-tests.feature and scaffold-theme-tests.feature. I'll have a look at those and see if they need reworking as well.

schlessera · 2020-02-03T14:16:18Z

Regarding the issues you're currently facing, here's what I would look into...

Extract making a query out into a separate shell function. Something along these lines:

sql_query() {
	$(mysql --user="$DB_USER" --password="$DB_PASS"$EXTRA -e "$1")
}

This way, all requests use the same credentials.

Then, use an actual query to drop and to create the database instead of the mysqladmin. With a query, you can specify the optional IF NOT EXISTS` to only create if needed:

sql_query "CREATE DATABASE IF NOT EXISTS $DB_NAME"

However, I have to admit, I don't know whether this will be the exact same thing in terms of user capabilities. I assume so, but a quick google didn't help to know for sure.

In terms of tests, I think you only need to adapt the install-wp-tests.feature file. I assume that the other two only assert that the file was created, but don't run it.

nylen · 2020-02-04T19:20:23Z

Then, use an actual query to drop and to create the database instead of the mysqladmin. With a query, you can specify the optional IF NOT EXISTS to only create if needed:

This will mean that the existing "skip-create-database" argument is not nearly as useful and might be worth removing entirely. It's also a more subtle change to the script's behavior: it will no longer fail / exit with a non-zero exit code when the database already exists.

Maybe not a big deal (hard to think of a case where this change would break something) but I think it is worth it to explicitly confirm that you're ok with the change here.

Fix an issue in which the mysql command line was not receiving or understanding the credentials being passed in

Add test for checking whether the database already exists, and how to handle it

tedsecretsource · 2020-02-05T16:57:56Z

In the end I left the conditional, moist code as I originally wrote it (sans a minor improvement to the line that checks for the existence of the database). I think there is a lot of opportunity for refactoring but I think that's a separate task as it requires a bit more understanding of all the contexts this file is used in to be done properly.

I modified an existing scenario to test for the correct response when responding affirmatively to the "drop existing database" prompt. I do wonder if I should add a test for a negative response to the prompt???

Please let me know what else, if anything, you'd like me to address (and don't be afraid to insist if I need to take your suggestions, @schlessera, I'm still learning how this works…)

tedsecretsource · 2020-02-05T17:15:50Z

I did want to respond to one aspect of this PR that I think should be different. I find the embedded if statements to be difficult to read / interpret. If you can live with executing a command that might return non-zero, the code could be simplified to:

# try to create the database
# if [ $? -gt 0 ]; then ask if we should drop the database and create it again; fi

This is a simplification of the code but I like the more direct approach: if the first command succeeds, just move along.

I guess I'm trying to understand where to draw the line between legibility and practicality. Any and all feedback is appreciated.

Simplified the condition for whether or not to recreate the database by moving the code into its own functions.

Modified one of the existing tests to test this feature. Specifically, this script should exit with return code 0, with text in both STDERR (the read prompt) and STDOUT (results of selected actions).

tedsecretsource · 2020-02-10T15:28:12Z

You'll also need to adapt the tests in https://github.com/wp-cli/scaffold-command/blob/master/features/install-wp-tests.feature

Done, and I think I now understand the difference between run and try.

schlessera

I'm fine with the code in its current state, but the "Leaving the database in place" needs to have a test as well.

schlessera · 2020-02-11T16:17:14Z

    When I run `WP_TESTS_DIR=/tmp/behat-wordpress-tests-lib phpunit -c {PLUGIN_DIR}/hello-world/phpunit.xml.dist`
    Then the return code should be 0

+    When I try `WP_TESTS_DIR=/tmp/behat-wordpress-tests-lib WP_CORE_DIR=/tmp/behat-wordpress /usr/bin/env bash {PLUGIN_DIR}/hello-world/bin/install-wp-tests.sh wp_cli_test_scaffold wp_cli_test password1 localhost latest < affirmative-response`


You should also add another test with the path where the user would not affirm, to ensure that this will indeed abort the database drop.

I'm fine with the code in its current state, but the "Leaving the database in place" needs to have a test as well.

Done. In all honesty, I feel like the tests are kind of superficial. For example, for the test to be robust, I should probably enter some data in the database, run the reinstall and respond NO to the prompt, and then test to see if the data is still in the database. Right now I'm just testing the output / logic in the script, not really the outcome of running it. Make sense?

Added a test with a negative response to the prompt to reinstall the database.

schlessera

Minor test string change, to make it clearer what is being tested AND test the DB name at the same time.

After that, we can merge this.

Adding more of the expected output to the assertion to reduce the possibility of false positives. Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>

Added parentheses around the database name in the test to make it match the actual output from the script.

schlessera · 2020-02-14T09:01:03Z

Thanks for the PR, @tedsecretsource !

…g-creation Fixed 228 - init script can be used repeatedly

Fixed 228 - init script can be used repeatedly

ca67057

Fixed in issue in which the init script would fail if the database was already created.

tedsecretsource requested a review from a team as a code owner January 24, 2020 15:01

nylen reviewed Jan 27, 2020

View reviewed changes

Added a condition so we don’t clobber existing DB

045bad1

Added a condition to test for the existance of an existing DB, and then asking for permission before deleting it.

schlessera added command:scaffold-plugin-tests Related to 'scaffold plugin-tests' command command:scaffold-theme-tests Related to 'scaffold theme-tests' command labels Jan 29, 2020

schlessera requested changes Jan 29, 2020

View reviewed changes

Comment thread templates/install-wp-tests.sh Outdated

Make confirmation test stricter

11f660f

Make the confirmation on whether or not to drop the existing database test for fewer possibilities (just y, Y, yes, Yes, and mixed case equivalents).

nylen reviewed Jan 30, 2020

View reviewed changes

Comment thread templates/install-wp-tests.sh

nylen reviewed Jan 30, 2020

View reviewed changes

Add missing creds

b7f1cd2

Added the missing credentials in the mysql call. Also changed the prompt in the drop db confirm to be something people are more used to seeing. Added an else case to handle instances in which the database doesn’t exists.

tedsecretsource added 2 commits February 5, 2020 16:52

Fix issue with mysql not getting creds

53d007e

Fix an issue in which the mysql command line was not receiving or understanding the credentials being passed in

Add test for checking existence of database

746ab7c

Add test for checking whether the database already exists, and how to handle it

tedsecretsource added 2 commits February 6, 2020 09:12

Refactor to improve legibility

0a7439c

Simplified the condition for whether or not to recreate the database by moving the code into its own functions.

Modify existing test for this feature

69b67da

Modified one of the existing tests to test this feature. Specifically, this script should exit with return code 0, with text in both STDERR (the read prompt) and STDOUT (results of selected actions).

schlessera requested changes Feb 11, 2020

View reviewed changes

Add test with negative response to prompt

caf740d

Added a test with a negative response to the prompt to reinstall the database.

schlessera requested changes Feb 13, 2020

View reviewed changes

Comment thread features/install-wp-tests.feature Outdated

Comment thread features/install-wp-tests.feature Outdated

tedsecretsource and others added 3 commits February 13, 2020 19:05

Make the test more verbose

e08a2ce

Adding more of the expected output to the assertion to reduce the possibility of false positives. Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>

Make test assertion more verbose

f13f55e

Adding more of the expected output to the assertion to reduce the possibility of false positives. Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>

Add parens to match actual script output

d0ef7f0

Added parentheses around the database name in the test to make it match the actual output from the script.

schlessera added this to the 2.0.9 milestone Feb 14, 2020

schlessera approved these changes Feb 14, 2020

View reviewed changes

schlessera merged commit 8b34e4e into wp-cli:master Feb 14, 2020

schlessera changed the title ~~Fixed 228 - init script can be used repeatedly~~ Deal with existing database in test init script Feb 14, 2020

yukihiko-shinoda mentioned this pull request Jun 18, 2020

Make use of $EXTRA connection settings in install_db() #269

Merged

danielbachhuber pushed a commit that referenced this pull request Nov 18, 2022

Merge pull request #249 from tedsecretsource/228-simplify-test-riggin…

4834680

…g-creation Fixed 228 - init script can be used repeatedly

Conversation

tedsecretsource commented Jan 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schlessera left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nylen commented Jan 30, 2020

Uh oh!

nylen Jan 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tedsecretsource commented Jan 30, 2020

Uh oh!

tedsecretsource commented Jan 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schlessera commented Feb 3, 2020

Uh oh!

nylen commented Feb 4, 2020

Uh oh!

tedsecretsource commented Feb 5, 2020

Uh oh!

tedsecretsource commented Feb 5, 2020

Uh oh!

tedsecretsource commented Feb 10, 2020

Uh oh!

schlessera left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schlessera left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

schlessera commented Feb 14, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nylen Jan 30, 2020 •

edited

Loading

tedsecretsource commented Jan 30, 2020 •

edited

Loading