WordPress.org

Make WordPress Core

Opened 16 months ago

Closed 6 months ago

#44233 closed enhancement (fixed)

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

Reported by: desrosj Owned by: desrosj
Milestone: 5.2 Priority: low
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

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.

Attachments (12)

43546.wpPrivacyDeleteOldExportFiles.diff (2.9 KB) - added by desrosj 16 months ago.
Unit tests for wp_privacy_delete_old_export_files from @allendav
43546.wpPrivacyGeneratePersonalDataExportFile.php (3.8 KB) - added by desrosj 16 months ago.
Work in progress from @allendav
43546.wpPrivacyProcessPersonalDataExportPage.diff (16.4 KB) - added by desrosj 16 months ago.
Unit-tests for wp_privacy_process_personal_data_export_page() from @birgire
43546.wpPrivacyGeneratePersonalDataExportFile.diff (9.0 KB) - added by allendav 15 months ago.
Covers everything coverable
44233.diff (30.7 KB) - added by desrosj 15 months ago.
44233.2.diff (36.3 KB) - added by birgire 15 months ago.
44233.3.diff (37.3 KB) - added by birgire 15 months ago.
44233.4.diff (37.9 KB) - added by desrosj 13 months ago.
44233.5.diff (38.2 KB) - added by desrosj 13 months ago.
44233.6.diff (35.8 KB) - added by iandunn 12 months ago.
44233.7.diff (29.4 KB) - added by desrosj 12 months ago.
44233.8.diff (2.1 KB) - added by desrosj 7 months ago.

Download all attachments as: .zip

Change History (50)

@desrosj
16 months ago

Unit tests for wp_privacy_delete_old_export_files from @allendav

@desrosj
16 months ago

Work in progress from @allendav

@desrosj
16 months ago

Unit-tests for wp_privacy_process_personal_data_export_page() from @birgire

#1 @desrosj
16 months ago

  • Type changed from defect (bug) to enhancement

#2 @desrosj
16 months ago

  • Priority changed from normal to low

Marking as low priority.

@allendav
15 months ago

Covers everything coverable

#3 @allendav
15 months ago

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

#4 @desrosj
15 months ago

  • Owner set to desrosj
  • Status changed from new to reviewing

#5 @desrosj
15 months ago

  • Keywords needs-refresh removed

@desrosj
15 months ago

#6 @desrosj
15 months ago

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.

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


15 months ago

#8 @birgire
15 months ago

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
15 months ago

@birgire
15 months ago

#9 @birgire
15 months ago

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_...".
Last edited 15 months ago by birgire (previous) (diff)

#10 @birgire
15 months ago

Somewhat related #44204

#11 @ocean90
15 months ago

  • Milestone changed from 4.9.7 to 4.9.8

4.9.7 has been released, moving to next milestone.

This ticket was mentioned in Slack in #core by pbiron. View the logs.


15 months ago

#13 @pbiron
15 months ago

Are these unit tests ready to land in 4.9.8?

#14 @desrosj
15 months ago

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.

This ticket was mentioned in Slack in #core by pbiron. View the logs.


14 months ago

#16 @pbiron
14 months ago

  • Milestone changed from 4.9.8 to 4.9.9

4.9.8 has hit RC, moving to 4.9.9.

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


14 months ago

#18 @desrosj
13 months ago

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

@desrosj
13 months ago

#19 @birgire
13 months ago

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.

Last edited 13 months ago by birgire (previous) (diff)

#20 @desrosj
13 months ago

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

Last edited 13 months ago by desrosj (previous) (diff)

@desrosj
13 months ago

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


13 months ago

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


12 months ago

#23 @iandunn
12 months ago

In 43640:

Privacy: Add test for wp_privacy_export_expiration filter.

Props desrosj.
See #44233.

@iandunn
12 months ago

#24 @iandunn
12 months ago

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.

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


12 months ago

#26 @desrosj
12 months ago

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.

@desrosj
12 months ago

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


12 months ago

#28 @pento
12 months ago

  • Milestone changed from 4.9.9 to Future Release

This ticket was mentioned in Slack in #core-privacy by webdevlaw. View the logs.


12 months ago

#30 @garrett-eclipse
8 months ago

  • Keywords has-unit-tests needs-refresh added

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

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


8 months ago

#32 @garrett-eclipse
7 months ago

  • Milestone changed from Future Release to 5.2

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

#33 @desrosj
7 months ago

  • Keywords needs-refresh removed
  • Resolution set to fixed
  • Status changed from reviewing to closed

This was fixed by [44786].

#34 @desrosj
7 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#35 @desrosj
7 months ago

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
7 months ago

#36 @desrosj
7 months ago

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

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


6 months ago

#38 @desrosj
6 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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

Note: See TracTickets for help on using tickets.