ext/mysqli: Work on making tests parallizable

[Girgias]

Bunch of cleanup in the first commits.

Beginning of moving tests with a new test_helper which are able to run in parallel. Will need to do this in multiple stages as I'm going to go insane trying to go through 400+ tests in one go.

[kamil-tekiela]

Can you merge the first 4 commits onto master? They look fine and they don't seem related to this PR much. These changes should have been made on their own.

[kamil-tekiela]

I will finish reviewing it tomorrow, but one thing stands out to me. I can't see $driver->report_mode = MYSQLI_REPORT_OFF; anywhere in your new file. Is it an omission? You can improve it by keeping only one connection-function, and in there first, enable error reporting and at the end, before returning, switch it off. This way you ensure that the connection will fail, but all other tests will still be responsible for their own error handling.

As an aside, the file renames are making the diff a little difficult to understand. Maybe apply file renames on master first?

[kamil-tekiela]

What exactly is the purpose of these two functions? Why not merge it all into my_mysqli_connect()?

[Girgias]

Part of the idea was to be able to "just" replace the current set of test files which this include instead of the mix of includes currently in place.

But for the ones I've touched I just kinda rewrote them on the way... so for the moment they are very pointless.

[Girgias]

Also this was copied from the current connect.inc file which specifies both versions (the non real and real versions).

As far as I could see from a glance, none of the tests actually specify custom args for the standard connection params so in the end I would probably just have everything use the default_mysqli_connect() function

[kamil-tekiela]

This already exists in my_mysqli_connect() so this is duplicated code.

[Girgias]

I will finish reviewing it tomorrow, but one thing stands out to me. I can't see $driver->report_mode = MYSQLI_REPORT_OFF; anywhere in your new file. Is it an omission? You can improve it by keeping only one connection-function, and in there first, enable error reporting and at the end, before returning, switch it off. This way you ensure that the connection will fail, but all other tests will still be responsible for their own error handling.

I didn't understand the purpose of this call initially. But I wonder if while at it it doesn't make sense to rewrite the tests to not need to deal with error handling and just fail with an exception. As I can imagine these tests being written prior to exception usage really being a thing.

As an aside, the file renames are making the diff a little difficult to understand. Maybe apply file renames on master first?

I'm not sure, how I would be able to do that frankly, as I figure out what the test is about while reading it for the refactor and then move it and rename it.

[Girgias]

@kamil-tekiela are those two lines actually necessary? As we are pretty inconsistent about using them and wonder if we could just simplify the tests by removing these lines?

[kamil-tekiela]

mysqli_select_db($link, $db); is unnecessary because it's already done in line above it: $link = my_mysqli_connect($host, $user, $passwd, $db, $port, $socket);

mysqli_query($link, "SET sql_mode=''"); is needed because of the test assumption: %s(19) "0000-00-00 00:00:00". Without we would be getting inconsistent test results depending on the DB version/setup. But it's only needed in this test.

[Girgias]

Does the engine need to be actually specified? Because again we are not very consistent in that regards...

[kamil-tekiela]

I have no idea. To my knowledge, the engine doesn't matter here. I am not entirely sure what this test actually tests.

[kamil-tekiela]

But I wonder if while at it it doesn't make sense to rewrite the tests to not need to deal with error handling and just fail with an exception. As I can imagine these tests being written prior to exception usage really being a thing.

That is true, but we need to test both modes. Therefore, best if each test would control this setting on its own. But you are right that the majority of tests were written before exception use was common. We could rewrite many of the tests, but it is menial work that doesn't seem that necessary.

[Girgias]

But I wonder if while at it it doesn't make sense to rewrite the tests to not need to deal with error handling and just fail with an exception. As I can imagine these tests being written prior to exception usage really being a thing.

That is true, but we need to test both modes. Therefore, best if each test would control this setting on its own. But you are right that the majority of tests were written before exception use was common. We could rewrite many of the tests, but it is menial work that doesn't seem that necessary.

Do we currently test the exception mode? A lot of the current failures checks seem to just bail (by calling exit()) when an error happens which is basically the behaviour of the exception mode?

[Girgias]

Okay @kamil-tekiela I think the tests are now cleaned-up and ready. I will just see if CI is finally green and then clean-up the DB link retrieval functions.

[kamil-tekiela]

Do we currently test the exception mode?

Well... we don't. Some tests do switch on exception mode and test both, but many do not. This is also why there were a number of bugs with missing exceptions.

I might try to do some cleanup to these tests now.

[kamil-tekiela]

While reviewing this PR I noticed a lot of issues with these tests. While I really love your PR and I hope we can merge it soon, can I ask you to put this aside for a week while I review these tests in more detail? I will try not to do too many conflicts. The current ones can be ignored when you rebase, as your changes will overwrite them.

[Girgias]

While reviewing this PR I noticed a lot of issues with these tests. While I really love your PR and I hope we can merge it soon, can I ask you to put this aside for a week while I review these tests in more detail? I will try not to do too many conflicts. The current ones can be ignored when you rebase, as your changes will overwrite them.

Sure, that's not a problem for me :)

[kamil-tekiela]

We can't do this. We don't actually know what the charset will be until we set it. We don't set the charset anywhere, so it could be anything. I also do not understand why on earth are we using substr here.
I'd suggest to rewrite this test and call set_charset twice with different values. After each call check if the charset actually changed.

[Girgias]

Should I delete this as we have ext/mysqli/tests/functions/mysqli_set_charset.phpt already?

[kamil-tekiela]

We also have ext\mysqli\tests\mysqli_character_set_name.phpt so I think it's safe to remove this one.

[kamil-tekiela]

Correct me if I am wrong, but I thought CLEAN sections are sandboxed, so this is unnecessary. This setting lasts only per execution of the script.

[Girgias]

I actually don't know if they are sandboxed, so you might be correct

[Girgias]

Just been told they do, I will remove this

[Girgias]

Currently away from my main desktop for a month so this PR might be on hold for that long. But I will go throughthe review comments

[Girgias]

@kamil-tekiela finally got round to this again, rebased and took all of my changes. Then started applying some of the review comments.

The only one I haven't applied just yet is the helper file one.

[kamil-tekiela]

Are you able to fix the failing tests? I think we should merge this in and then continue fixing in next PRs. This is too big PR and I am having really hard time reviewing it properly. The more I look at them the more I don't like these tests.

[Girgias]

Are you able to fix the failing tests? I think we should merge this in and then continue fixing in next PRs. This is too big PR and I am having really hard time reviewing it properly. The more I look at them the more I don't like these tests.

I'll merge if the build is green