MDEV-28914 Extend MTR command "copy_file" to support recursive copy and wildcard

[PhysicsTing]

• [x] The Jira issue number for this PR is: MDEV-28914

Description

A few commands have been missing in MariaDB MTR compare to MySQL MTR:

• "force_cpdir" is useful when trying to duplicate the data directory for destructive tests
• "copy_files_wildcard" is useful when trying to copy all files related to a specific table to another location

However instead of introducing these commands to MariaDB, it is better to extend the existing command "copy_file" to support both recursive copy and wildcard.

With this commit, "copy_file" can be used in similar way as "cp" in bash.

• Copy a regular file: copy_file file1 file2;

• Copy a directory: copy_file dir1 dir2 -r;

• Copy with wildcard: copy_file file.* dir2;

How can this PR be tested?

All existing test cases in main suite including the ones that test mysqltest itself passed. New test cases added into mysqltest.test regarding for the new commands.

Basing the PR against the correct MariaDB version

• [x] This is a new feature and the PR is based against the latest MariaDB development branch

Backward compatibility

New commands added will not compromise any backward compatibility

Copyright

All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[ottok]

This PR was discussed in https://jira.mariadb.org/browse/MDEV-28914

Latest comment by @vuvova

force_cpdir — right, I see. Perhaps, then, copy_files_wildcard should copy directories recursively then? In fact, I'd personally prefer to have only one command, copy_files that does wildcards and copy recursively, but someone has already added copy_files_wildcard instead of extending the existing command. Let's not do that again?

assert — well, nothing helps against somebody who is determined to update the results, I've seen people updating asserts too. In my opinion the main goal of a .test file (besides actually, verifying some behavior) is good debug-ability, when a test fails, it should be as easy as possible to understand what's going on. A diff between expected and actual results help much more than an abort like in source include/assert.inc. I sometimes use the pattern like

select count(col) as 'must be 1' from t1;

that makes it somewhat more difficult to update the result and not notice the incongruity.

[dr-m]

I agree with @vuvova that the assert may be unnecessarily and frustratingly hiding vital information in case there is an intermittent failure. In some tests, I have seen constructs like

SELECT COUNT(*)=0 FROM table_that_is_expected_to_be_empty;
SELECT COUNT(*)=1 FROM table_that_is_expected_to_contain_one_row;

This is very unhelpful. If a table is expected to contain a small number of rows, then it would be better to do one of the following, so that in case the result differs from the expected, we will have some more details and not just "it differed".

SELECT * FROM table_that_is_expected_to_be_empty;
SELECT pk,and_other_non_blob_columns FROM table_that_is_expected_to_contain_one_row;

When it comes to the assert construct, it seems to make the Windows tests fail. At least that should be addressed before this can move forward.

Note: While I agree with @vuvova that the assert construct does not seem too useful, I do not strongly oppose adding it if it really helps some testing. I might oppose extensive use of it in tests that are intended to run on our CI systems.

[PhysicsTing]

Thank you @dr-m for your comment. Indeed it's a good point that assert() or assert-like commands tend to hide information. If there must be a counter argument, I would say that in some cases hiding information is desirable as it highlights what's important and what's not. However it is indeed a trouble if people start abusing it. I don't want to introduce something that we end up having to discourage people from using it, so I will leave the assert() command out of this PR.

For the "force_cpdir" command, I think it's a good idea to just use one command "copy_file" for all (support both recursive and wildcard).

"but someone has already added copy_files_wildcard instead of extending the existing command" No. I ported it from MySQL with this current PR. Not too late to remove : )

So the next revision will not add any new command, but instead, extend "copy_file" to support both recursive copy and wildcard.

[vaintroub]

assert() tends to hide information, therefore some testing frameworks invented asserts that do not hide information, instead presenting it in a well readable form. E.g assertEquals in JUnit, which , when ported , would look something like

assertEquals(select count(*) from seq_1_to_6, 5)

and fail with something like expression 'select count(*) from seq_1_to_6' returned 6, expected 5" at line 1, file mytest.test Personally I would prefer the assertEquals to repetitive SELECT 2 as EXPECT_1, that would fail with a diff.

[PhysicsTing]

I pushed a new version to extend existing command copy_file to support recursive copy and wildcard instead of introducing new commands as I did in the previous version. Also re-based to the latest branch 10.11. assert command in previous version is removed.

[ottok]

Please re-review updated version

[robinnewhouse]

Hi @LinuxJedi, @vuvova, would you be able to re-review the latest changes in this PR? Thank you.

[PhysicsTing]

Sorry @LinuxJedi I missed your comment about symlink i the latest version. Let me test the scenario.

[PhysicsTing]

Hi @LinuxJedi I refreshed this PR to include additional comment and MTR for copying symlink. Please let me know if there are more concerns. Thanks!

[PhysicsTing]

I have made a few adjustments following the code standards (space before "=" sign, curly braces on new line, 80 char max), and rebased to 11.3. Please let me know if I missed anything.

A few lines has 83 chars because it would look very weird if I wrap them just due to 3 letters. Let me know if the 80 char standard is a hard limit.

Thanks fin advance for reviewing.

[ottok]

@PhysicsTing Please rebase one more time and mark discussions as "resolved". I just clicked the button to request re-review from Andrew.

[PhysicsTing]

I see 11.4 branch is till the default branch so I kept it, but rebased to latest HEAD.

Need one more update to use dynstr_append_mem instead of dynstr_append. Will push again shortly.

[PhysicsTing]

3 failures all passed on re-try

• buildbot/amd64-debian-10-last-N-failed
• buildbot/amd64-debian-11-debug-ps-embedded
• buildbot/amd64-ubuntu-2204-debug-ps

1 failed test case is know to be unstable. I have seen the same error on other PRs

• galera_3nodes.galera_gtid_consistency

Pleas help review and merge if possible. Thanks in advance!