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

Make BerlinDB easy to be unit tested with WordPress test suite #104

Open
engahmeds3ed opened this issue May 20, 2021 · 7 comments · Fixed by #110
Open

Make BerlinDB easy to be unit tested with WordPress test suite #104

engahmeds3ed opened this issue May 20, 2021 · 7 comments · Fixed by #110
Assignees

Comments

@engahmeds3ed
Copy link

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.

@JJJ
Copy link
Collaborator

JJJ commented May 25, 2021

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! 💫

@engahmeds3ed
Copy link
Author

Thanks @JJJ

I will create a PR tonight for this one.

JJJ added a commit that referenced this issue May 28, 2021
This change improves compatibility with temporary tables used in PHPUnit test suites.

Fixes #104.
@JJJ JJJ closed this as completed in #110 May 28, 2021
JJJ added a commit that referenced this issue May 28, 2021
This change improves compatibility with temporary tables used in PHPUnit test suites.

Fixes #104.
@engahmeds3ed
Copy link
Author

@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.

@JJJ JJJ reopened this May 28, 2021
@JJJ
Copy link
Collaborator

JJJ commented May 28, 2021

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

@engahmeds3ed
Copy link
Author

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.

@JJJ
Copy link
Collaborator

JJJ commented May 28, 2021

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:

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

@JJJ
Copy link
Collaborator

JJJ commented May 28, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants