Problem/Motivation

We need tests for user status, test each status in its own file:
1.- A user to be pre-notified
2.- A user to be purged but was already pre-notified
3.- A user that should not be touched or email be sent to (an active user)

Each of above tests needs to:
1.- Test in both the form and cron (as it is also now)
2.- Use only the "Disable the account and keep its content." cancellation

We also need tests for the cancelling method:
1.- a user to be purged (pick a status, doesn't matter)

Each of above tests needs to:
1.- Test in both the form and cron (as it is also now)
2.- Test that the user and his content is handled correctly
3.- We also need to test the email options:

Even though we test some email being send in the first batch of tests, those tests needs to test:
1.- A user to be pre-notified
2.- One user to be purged, was already pre-notified, the cancellation method needs to be one that doesn't delete the user

Each of above tests needs to:
1.- Test in both the form and cron, calling them multiple times to detect spam
2.- Use only the "Disable the account and keep its content." cancellation
3.- Don't test for user status and content, we already test that in prev tests
4.- Also change the setting to disable the emails, run the cron and form again, check that the emails where not sent

Then we test what remains:

PurgeOnCronTest.php, one user to be purged, cron enabled, assert was purged, disable cron, assert was not purged
DisregardInactiveBlockedUsersTest.php, one user active user one inactive user, test the on/off of the setting
RoleLimitationTest.php, one admin user, one authenticated, toggle the setting, assert that once they are pruged, then they are not.
SettingsFormTest.php, the current form test.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

selvira created an issue. See original summary.

vintimpe made their first commit to this issue’s fork.

donquixote’s picture

Hi!
Quick intermediate feedback:
The branch contains way too many commits that go back and forth.
It removes a good number of the old tests and introduces new tests, but it is not clear how the new tests replace the old tests.

I strongly recommend to identify the main ideas we are trying to accomplish, and to create "change packages" with clear focus.
This can either be a big PR where the commits are well-organized and each commit has a clear focus, OR separate issues and merge requests.

We should always have separate commits, perhaps separate issues, for:
- Code style changes.
- Refactoring: Renaming classes, extracting shared code, etc, _without_ changing test coverage or behavior.
- Behavior changes, accompanied by test changes.
- New tests to increase coverage.
- New features, accompanied by new tests.

If there is no continuity at all between old tests and new tests, then perhaps the best we can do is to squash all the test changes as "Rewrite tests.". But then it is completely unclear from the history why the old tests were removed.

-----

EDIT: I have some ideas now, how to organize these tests :)

donquixote’s picture

I made a new PR which is completely different.
It uses kernel tests and data providers, and covers a lot more with less code.

Perhaps we can keep the old tests in addition for now, just to be sure that functionality does not depend on KernelTest, but also works in BrowserTest.

(I wonder if testbot will run for this?)

donquixote’s picture

Status: Active » Needs review
donquixote’s picture

Btw we could also convert these back into browse tests, but they would be much slower, or we would have to reduce the datasets from the data providers, causing decreased coverage.

Andras_Szilagyi made their first commit to this issue’s fork.

  • donquixote committed 241c6f5 on 8.x-3.x
    Issue #3246399: Testing: Add PurgeMethodTest + ChangedNodesTrait.
    
  • donquixote committed 556b417 on 8.x-3.x
    Issue #3246399: Testing: Add PurgeConditionTest + base class and traits.
    

Andras_Szilagyi’s picture

Andras_Szilagyi’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.