Problem/Motivation

In FlagSimpleTest, the doUserDeletionTest() method tests the deletion of flags when a user is deleted. We are trying to get rid of FlagSimpleTest.

Proposed resolution

Create a new test class. This need not be user centric, but test the deletion of both user and their flags, and an entity and its flags. Remove deUserDeletionTest() from FlagSimpleTest.

Remaining tasks

Create patch.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#14 interdiff-2702911-12-14.txt535 bytesmartin107
#14 deleteTest_2702911.14.patch5.5 KBmartin107
#12 deleteTest_2702911.12.patch5.42 KBsocketwench
#10 deleteTest_2702911.10.patch5.13 KBsocketwench
#4 deleteTest_2702911.4.patch6.57 KBmartin107
#2 deleteTest_2702911.2.patch6.27 KBsocketwench
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

socketwench created an issue. See original summary.

socketwench’s picture

socketwench CreditAttribution: socketwench at FFW commented
Status: Active » Needs review
FileSize
deleteTest_2702911.2.patch6.27 KB

This simply moves the test from FlagSimpleTest to a new test class. It does not add node deletion or flag deletion tests.

martin107’s picture

martin107 CreditAttribution: martin107 commented
Assigned: Unassigned » martin107

Needs Reroll ... I am working on this now.

martin107’s picture

martin107 CreditAttribution: martin107 commented
Assigned: martin107 » Unassigned
FileSize
deleteTest_2702911.4.patch6.57 KB

Here we go.

socketwench’s picture

socketwench CreditAttribution: socketwench at FFW commented 
+++ b/src/Tests/FlagDeletionTest.php
@@ -0,0 +1,103 @@
\ No newline at end of file

Missing newline at the end of FlagDeletionTest.php. Other than that, it looks good so far. I'd like to add test cases for node deletion and then flag deletion too.

martin107’s picture

martin107 CreditAttribution: martin107 commented
Status: Needs review » Needs work
martin107’s picture

martin107 CreditAttribution: martin107 commented
Status: Needs work » Closed (duplicate)

makes sense to me.

socketwench’s picture

socketwench CreditAttribution: socketwench at FFW commented
Status: Closed (duplicate) » Needs work
martin107’s picture

martin107 CreditAttribution: martin107 commented

Sorry looks like I closed the wrong issue.

socketwench’s picture

socketwench CreditAttribution: socketwench at FFW commented
Status: Needs work » Needs review
FileSize
deleteTest_2702911.10.patch5.13 KB

Now that I think about it, there's not much reason to make a separate test here. Much of what I had in mind is already covered by FlagCountsTest. Let's just add the user test to the counts test.

Status: Needs review » Needs work

The last submitted patch, 10: deleteTest_2702911.10.patch, failed testing.

socketwench’s picture

socketwench CreditAttribution: socketwench at FFW commented
FileSize
deleteTest_2702911.12.patch5.42 KB

Added missing schema statement.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs work » Needs review
martin107’s picture

martin107 CreditAttribution: martin107 commented
FileSize
deleteTest_2702911.14.patch5.5 KB
interdiff-2702911-12-14.txt535 bytes

Conceptually it is good to see those test move into a kernel test , and FlagCountsTest is the obvious choice.

Practically speaking the patch looks good, all the relevant testing has been transferred cleanly.

I have only rerolled this patch - so plus one from me - I regard this as RTBC

I have only the smallest of polishes to make

FlagCountTest::testUserDeletion() has no annonation.

  • socketwench authored 9b39128 on 8.x-4.x 
    Issue #2702911 by socketwench, martin107: Moved user deletion test from...
socketwench’s picture

socketwench CreditAttribution: socketwench at FFW commented
Status: Needs review » Fixed

Thanks for the fixes, Martin!

Status: Fixed » Closed (fixed)

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