too many tests create a flag with the form [#2558503]

While working on another issue, I've found there's a lot of tests that create a flag using the form. This seems excessive to me. Admittedly, each link type plugin needs to work with the flag form to check its own stuff is there, but aside from that, the flag form only needs to be tested once. The rest of the time, flags should be getting created/edited programmatically so tests run quicker.

Comment	File	Size	Author
#3	2558503.2.refactorTests.patch	5.69 KB	socketwench
#3

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

27 August 2015 at 11:00

joachim created an issue. See original summary.

Comment #2

socketwench CreditAttribution: socketwench as a volunteer commented 1 September 2015 at 03:19

~~From my poking around, there's only three places where this issue would apply:~~

~~FlagFieldEntryTest~~
~~FlagServiceTest~~
~~FlagEnableDisableTest~~

There are larger refactoring problems elsewhere. FlagServiceTest creates the flag programmatically without using FlagTestBase::createFlag(). FlagConfirmFormTest, FlagFieldEntryTest, and FlagSimpleTest call drupal_post_form() directly instead of using FlagTestBase::createFlagWithForm(). Many of these classes don't take advantage of FlagTestBase now that it's in the repo. These will need new issues.

Comment #3

socketwench CreditAttribution: socketwench as a volunteer commented 1 September 2015 at 03:53

File	Size
2558503.2.refactorTests.patch	5.69 KB

Did a few refactors, by no means finished.

Comment #4

joachim CreditAttribution: joachim commented 1 September 2015 at 08:12

> There are larger refactoring problems elsewhere

Agreed! After filing this, I had another think and figured we need a meta-task to think about the complete picture of the tests we have, but life intervened and I've not filed it yet.

Comment #5

joachim CreditAttribution: joachim commented 1 September 2015 at 21:25

+++ b/src/Tests/FlagConfirmFormTest.php
@@ -108,7 +92,7 @@ class FlagConfirmFormTest extends WebTestBase {
-    $this->drupalPostForm(NULL, $edit, t('Create Flag'));
+    $this->flag = $this->createFlagWithForm('node', $edit, 'confirm');

We shouldn't be creating the flag with the form at all here -- the test isn't about the Flag admin UI, so we should create the flag programatically.

Comment #6

socketwench CreditAttribution: socketwench as a volunteer commented 2 September 2015 at 14:04

We shouldn't be creating the flag with the form at all here -- the test isn't about the Flag admin UI, so we should create the flag programatically.

Then when where do we test the Confirm Form link type form?

Comment #7

joachim CreditAttribution: joachim commented 7 September 2015 at 15:41

Ah right, yes, sorry. I see what you mean -- we need to test the admin form elements for the link type we're testing.

I'd prefer it if the patch were split up -- with tests I feel it's very easy to remove too much and then never notice it's gone because tests pass. Also, the time I have to review these days is usually too early in the morning or too late at night :/ so simpler patches are preferable for reasons of limited brain power too!