Add missing unit tests for exporting personal data by username or email address

[desrosj]

This ticket picks up where #43546 left off to add missing tests for the functionality introduced and to keep when the enhancement and tests were introduced clear.

See #43546. So far, props @birgire, @allendav.

[desrosj]

Unit tests for wp_privacy_delete_old_export_files from @allendav

[desrosj]

Work in progress from @allendav

[desrosj]

Unit-tests for wp_privacy_process_personal_data_export_page() from @birgire

[desrosj]

Marking as low priority.

[allendav]

Covers everything coverable

[allendav]

@desrosj I just added a final pass at unit tests for wpPrivacyGeneratePersonalDataExportFile - I think it now covers everything that can be covered now.

[desrosj]

44233.diff​ combines the uncommitted tests from the three patches into one and contains the following changes:

• Added a test in wpPrivacyDeleteOldExportFiles.php that tests file deletion when the expiration time period is filtered.
• Changed @since tag to 4.9.7 in Tests_Privacy_WpPrivacyProcessPersonalDataExportPage and Tests_Privacy_wpPrivacyGeneratePersonalDataExportFile classes. Not sure if this @since should match the function or test introduction version (thinking the test).
• Removed variable declarations for $filtered_response in test methods that did not utilize the variable in wpPrivacyProcessPersonalDataExportPage.
• Move register_custom_personal_data_exporters() closer to the top of the class Tests_Privacy_WpPrivacyProcessPersonalDataExportPage class where it is used.
• Add a skip condition to Tests_Privacy_wpPrivacyGeneratePersonalDataExportFile->setUp()
• Replace @unlink() call with a wp_delete_file() call.
• Move $exception_was_thrown variables in each test method within the Tests_Privacy_wpPrivacyGeneratePersonalDataExportFile class to a class property to avoid repeating in every method.
• Added checks that the export file was actually created for success scenario test methods in the Tests_Privacy_wpPrivacyGeneratePersonalDataExportFile class.
• Log the created export file on the action in a class property within the Tests_Privacy_wpPrivacyGeneratePersonalDataExportFile class and use it to test that the file exists after calling the function.

I set up a ​branch on my `wordpress-develop` fork to show the tests passing.

Remaining items:

• There are some PHP 5.2 failures that I am looking into.
• The test_wp_privacy_generate_personal_data_export_file_detects_cannot_create_index() and test_wp_privacy_generate_personal_data_export_file_detects_cannot_write_html() test methods are failing locally for me but passing on Travis. I think this is permissions related. Going to look into a more system independent way to set up these test methods.

[birgire]

Thanks for the update @desrosj

44233.2.diff​ is an udpate for Tests_Privacy_WpPrivacyProcessPersonalDataExportPage.

It uses the same approach as in Tests_Privacy_wpPrivacyGeneratePersonalDataExportFile, using parts of WP_Ajax_UnitTestCase, instead of extending it, to get the output of wp_send_json_error( '...' ), from a custom ajax die handler. It further uses annotations, introduces test_function_should_send_error_when_invalid_request_action_name() and enhances the inline documentation.

ps: I currently don't have PHP 5.2, to look into the 5.2 issues from Travis, but it's not obvious to me just by looking at the code :) First I thought it might be related to empty checks on functions, but that's supported by PHP 5.5+. Might need to peek into the relevant assert functions of PHPUnit to see how it's defined.

[birgire]

It's not easy to test wp_privacy_generate_personal_data_export_file(), so thumbs up to @allendav.

I tested Tests_Privacy_wpPrivacyGeneratePersonalDataExportFile successfully, but noticed that it left behind some files and a subdirectory, in the privacy export directory.

44233.3.diff​ includes suggestions that:

• Renames the class to Tests_Privacy_WpPrivacyGeneratePersonalDataExportFile.
• Avoids the exec/shell execution to remove the privacy export directory.
• Introduces the remove_exports_dir() helper method, that uses the list_files() function that already exists in core. First I did consider expanding and using the remove_added_uploads(), files_in_dir(), scan_dir() and delete_folders() methods of WP_UnitTestCase. That should be possible, but e.g. the RecursiveDirectoryIterator::SKIP_DOTS is not supported in PHP 5.2.
• Makes an extra check on the privacy export directory path, to avoid it's corrupted by filters, when it's removed.
• Adds extra checks to skip tests.
• Makes sure all test files/directories are removed, before and after each test run.
• Uses the static $exports_dir to store the privacy export directory path.
• Simplifies the name of the test methods, use "test_function_..." instead of "test_wp_privacy_generate_personal_data_export_file_...".

[birgire]

Somewhat related #44204

[ocean90]

4.9.7 has been released, moving to next milestone.

[pbiron]

Are these unit tests ready to land in 4.9.8?

[desrosj]

Remaining items on this ticket (besides refreshing it for 4.9.8):

• The tests are failing in PHP 5.2. This needs to be investigated and fixed.
• Two of the new test methods are failing locally for me in VVV, but they are passing in Travis. This most likely has to do with the way the tests are creating files and directories and setting their permissions. This needs to be fixed.

[pbiron]

4.9.8 has hit RC, moving to 4.9.9.

[desrosj]

44233.4.diff​ fixes the failing tests in PHP 5.2. In PHPUnit 3.6 (which the PHP 5.2 build in Travis uses), the @expectedException tag cannot expect a general exception. The test_function_detects_cannot_write_html() and test_function_detects_cannot_create_index() functions are still failing locally for me, though.

Here is the passing PHP 5.2 build: ​https://travis-ci.org/desrosj/wordpress-develop/jobs/415579815

[birgire]

Awesome @desrosj, thanks for digging into the 5.2 case. Good to know about the general exception support.

The test_function_detects_cannot_write_html() and test_function_detects_cannot_create_index() functions are still failing locally for me, though.

It runs successfully on my Linux node.

It seems that the mode of:

mkdir( $pathname, $mode ) 

isn't supported on Windows.

​https://secure.php.net/manual/en/function.mkdir.php

Could that be the reason?

Maybe skip those tests for Windows that depend on setting the mode?

Wonder if there are similar Windows challenges regarding chmod()?

If not, could we chmod after mkdir() above?

ps: in future it would be nice to be able to make these filesystem checks virtually (vfs).

Then there is WP_Filesystem_MockFS for WP_Filesystem.

[desrosj]

I did a poor job summarizing the conditions the tests fail locally under in my last comment. Sorry for that!

Those two test mothods are failing locally on Windows and MacOS. I am using VVV.

Ideally, the filesystem should be mocked for these tests. But, as far as I can tell, this is blocked by #44204 (the privacy functions currently do not utilize the WP Filesystem API). It might also be nice to utilize ​vfsStream for these and similar tests, but that lacks PHP 5.2 support. These tests could easily be skipped on the PHP 5.2 build, though.

44233.5.diff​ avoids the mode parameter of mkdir() (was a remnant of me trying different approaches) and skips the two test methods that are failing if the directory permissions are not able to be changed correctly.

One thing to point out with this approach is that if something changes in Travis and the tests are skipped because permissions are not changed correctly, the assertions would silently stop being tested.

​Latest passing build

[iandunn]

In 43640:

Privacy: Add test for wp_privacy_export_expiration filter.

Props desrosj.
See #44233.

[iandunn]

r43640 commits the changes to wpPrivacyDeleteOldExportFiles.php. The other two files look pretty good, but need a few more changes.

I did some minor cleanup in 44233.6.diff​:

• Minor coding standards issues
• Removing comments that didn't feel necessary because the code was descriptive on its own.
• Prefixing the (global) function name with :: in the @covers annotation, so PHPUnit's code coverage can find the function

The one big change I made was to remove the try...catch blocks (and related code) in favor of the existing WPDieException / setExpectedException() pattern used elsewhere in the suite. The avoids the problem mentioned in comment:18, and makes the code smaller and easier to read. setExpectedException() was deprecated in PHPUnit 7, but Core already has a shim for it that calls expectException(). This should be back-compat with PHPUnit 3.7 and forward-compat with 7.x, but so far I've only tested on 6.5.

I left a few questions in the diff, marked with todo. @desrosj, @birgire, can y'all take a look at those? There's mostly small docs issues. The only big change I think this might need is to refactor wpPrivacyProcessPersonalDataExportPage.php to use a @dataProvider, to avoid having a ton of tests with duplicated code.

[desrosj]

Thanks, @iandunn! See my responses below which are reflected in 44233.7.diff​ .

Tests_Privacy_WpPrivacyProcessPersonalDataExportPage

How is $request_email different from $requester_email?

$request_email appears to left over from a patch refactor. I removed it because it was not used anywhere ($requester_email was used instead in all test methods).

Why is this (wp_mail_from hook) removed here instead of in test_function_should_send_error_on_last_page_of_last_exporter_when_mail_delivery_fails()?

I removed all filter and action related removing in the tearDown() method because parent::tearDown() already handles these.

Also in this class:

• Removed self::$send_as_email. This is only ever true or false. I created a data provider for methods that should test when an email is and is not sent.

Tests_Privacy_WpPrivacyGeneratePersonalDataExportFile

What problem is caused by a plugin intentionally changing the path to the folder?

The tearDown() method will delete the export folder after each test method. I think @birgire had added this, but I think the intent was to prevent a non-default export folder from being removed when running the tests on an install where a plugin was filtering the directory. I removed these checks in my patch. If @birgire can point to a reason this should stay, we can add the checks back.

Is it unsafe to assume that $exports_dir is a folder? What would cause it to be a file?

The exports directory will be a file after the test_detect_cannot_create_folder() test method which verifies an error is returned when the export directory is a file. I added an inline comment above that conditional for context. It also could be a file if an incorrect value is returned to the wp_privacy_exports_dir filter.

Also in the patch:

• Removed _function_ from all methods that had test_function_does_something for more precise naming.
• Created a DRY _setup_expected_failure() helper method to set up a test method to expect an exception.
• Removed @since tags on test methods in favor of @ticket tags.

[garrett-eclipse]

Thanks @desrosj this applies cleanly with all tests passing. It will just need a version change when it's milestoned.

[garrett-eclipse]

@desrosj this still applies cleanly and passes on tests. Moving into 5.2, could you refresh with version numbers to match.

[desrosj]

This was fixed by [44786].

[desrosj]

Prior to committing, all tests were passing locally (using VVV), and on a test branch on Travis.

After committing, two of the hosts reporting test suite results ​began ​failing. Digging into it, the following two test methods that were finicky earlier in this ticket's history were causing the failures:

• test_detects_cannot_create_index()
• tests_detects_cannot_write_html()

I am going to remove those two methods until a new way to test those scenarios can be determined.

[desrosj]

In 44792:

Privacy: Remove two test methods that fail on certain configurations.

The test_detects_cannot_create_index() and tests_detects_cannot_write_html() test methods are prone to failure under certain configurations, as discovered by the hosts reporting back the test suite results. This removes those two methods until a better approach to testing those scenarios can be created.

Partial revert of [44786].
See #44233.

[desrosj]

44233.8.diff​ re-adds the test methods that were failing in order to debug.

[desrosj]

Closing this out. Opened #46577 to further investigate the issues with file permission related tests.