core icon indicating copy to clipboard operation
core copied to clipboard

Make BerlinDB easy to be unit tested with WordPress test suite

Open engahmeds3ed opened this issue 4 years ago • 7 comments

As we know that WP test suite will create any not core tables as temporary tables.

So try this MySQL snippet

CREATE TEMPORARY TABLE wp_test_credits(
    customerNumber INT PRIMARY KEY,
    creditLimit DEC(10,2)
);

this is what exactly WP unit tests framework does.

Then here https://github.com/berlindb/core/blob/master/table.php#L335

You are trying to run the following mysql query

SHOW TABLES LIKE '%wp_tesssst_credits%';

and this show tables isn't working with temporary tables so the exists method will return false forever in tests, which affects some other methods.

I think if you used the following query

SHOW CREATE TABLE wp_tesssst_credits;

this should work with all tables (temp or normal)

If you agree on the idea, I can open PR for it to validate.

engahmeds3ed avatar May 20 '21 17:05 engahmeds3ed

Hey @engahmeds3ed, thank you for creating this issue. 🙏

I've researched it, concluded it is valid, and your suggested code change is accurate. 🚀

I don't love it, but it's just MySQL being MySQL I suppose. 😅

A pull request would be greatly appreciated! 💫

JJJ avatar May 25 '21 14:05 JJJ

Thanks @JJJ

I will create a PR tonight for this one.

engahmeds3ed avatar May 25 '21 16:05 engahmeds3ed

@JJJ I can see that you created a PR for that one. Sorry for being late but I was investigating this one as I found that if we used

SHOW CREATE TABLE wp_tesssst_credits;

and the table wasn't existing, it would throw an error to your log so I needed to suppress the errors something like that

$query    = "SHOW CREATE TABLE %1s";
$prepared = $db->prepare( $query, $this->table_name );

$suppress = $db->suppress_errors;
$db->suppress_errors(true);

$result   = (string) $db->get_var( $prepared );

$db->suppress_errors($suppress);

but I need to test that one extensively.

plz try this PR with an existing table and not an existing one to verify my finding here.

engahmeds3ed avatar May 28 '21 03:05 engahmeds3ed

Good news! You are correct, again. 😁

I would prefer to not juggle error suppression anywhere, but it may be the easiest thing to do here.

(Related to your code suggestion specifically, I believe it's not necessary to prepare the table name, as WordPress will handle it inside of WPDB.)

JJJ avatar May 28 '21 04:05 JJJ

I will try doing another approach today and by this weekend I think I will solve that, Yes I will do my best not to use this suppress.

Thanks, Boss.

engahmeds3ed avatar May 28 '21 04:05 engahmeds3ed

Suppressing errors doesn't work extremely well when using plugins like Query Monitor, which suppress the errors by design but show them inside of its own UI.

I was testing both with and without it, but ironically was breaking at a time where QM was always hiding them. This was my error, and I've learned from it. 😓

In my research, it appears that there is no way to reliably show a table regardless of it being temporary, not even a clever one.

For your review:

  • https://dev.mysql.com/doc/refman/8.0/en/temporary-table-problems.html
  • https://dev.mysql.com/doc/refman/5.7/en/innodb-information-schema-temp-table-info.html
  • https://lists.mysql.com/mysql/215317

Suggest to revert the previous PR, and consider a new approach. Perhaps related to #105?

JJJ avatar May 28 '21 05:05 JJJ

Reverted from master and release/2.0.0 branches for now. 👍

JJJ avatar May 28 '21 05:05 JJJ