Reduce the number of calls to FlagTestBase::createFlagWithForm() to make PHPUnit conversion easier

Break things into small steps.... this is a good response to

#2751053: Convert Simpletests to PHPUnit

6 function calls to convert.... I want to see how this shakes out..

> While this was not a problem for Simpletests, it poses a problem for converting more tests to PHPUnit. createFlagWithForm() relies on drupalPostAJAXForm()

Whoa, PHPUnit can't do AJAX form submissions? That's going to be a problem. How is core handling this?

createFlagFromArray in simplistic terms looks very similar in outcome to createFlagFromForm

both functions create a flag - assert what is created is an instance of flag and the return the object.

So I have made createFlagFromArray more flexible - it takes 2 extra params

So here is my first go - I am just going to throw this at testbot and then cleanup issues as they arise.

The intention is to change createFlagFromArray as little as possible - what this means in practise is that I have had to swap the order of the method parameters as I converted them.

Lets hope I did not break all the things and go bang.

This is just a partial fix ... I am just trying to work in small self contained sections.

1) In a few places it it not longer appropriate to look for "Flag @this_label has been added" type messages

2) createFlagFromArray the default array needs to expand if the link type is 'field_entry'

more tomorrow.

Just small steps - not there yet

1) I am unwinding a kink I introduced.

The parameters I added to createFlagFromArray were just preconfiguring the defaults -- why not override the defaults.... doh

2)I just want to comment on something that could fool us into a feeling of security in the future.

regarding the switch-case statements to set extended defaults in createFromFlag()

It is good that by default unset fields get stuffed with random data as a testing best practice..

but it is not yet complete and needs to be kept in sync with changes to flag/config/flag.schema.yml

In a future iteration I may cut it out and move it to a separate issue. or find a way of automatically keeping things in sync- not sure yet.

Did I say I like to overcomplicate things!!!!

1. +++ b/src/Tests/FlagTestBase.php
   @@ -144,17 +144,23 @@ abstract class FlagTestBase extends WebTestBase {
   -  protected function createFlagFromArray(array $edit) {
   +  protected function createFlagFromArray(array $edit, $entity_type = 'node', $link_type = 'reload') {
   

   Not sure about this change.

   The point of having individual params is to make it easier to create flags when their configuration is standard -- eg, a node flag with a reload link type -- because you don't have to figure out what goes where inside $edit. But if $edit comes first, you have to specify it anyway...

   So we could put $edit at the end, but that makes this patch a lot more disruptive, and means that when you want a flag with unusual config, you need to give NULL for the first few params.

   Or we leave createFlagFromArray() as is, and add a helper method that wraps around createFlagFromArray(), called something like createFlagBasic($entity_type, $link_type).

2. +++ b/src/Tests/LinkTypeAjaxTest.php
   @@ -49,7 +49,7 @@ class LinkTypeAjaxTest extends FlagTestBase {
   -    $this->flag = $this->createFlagWithForm('node', [], 'ajax_link');
   +    $this->flag = $this->createFlagFromArray( [], 'node', 'ajax_link');
   

   Are these removals not going to lose us testing of the admin UI for the particular link type?

Thats for the review of something that is a work in progress.

I am just makeing notes for the next iteration.

#16.1

I agree helper functions are good - instead of createFlagBasic() the existing helper method createFlag() can cover all cases I think
So I am going to move away from createFlagFromArray where possible.

#16.2

Good catch - thanks ... to my mind this issue just to lay the groundwork for the parent issue
I don't think that is a something we should solve here ... so I am going to restore createFlagWithForm()
and back out the changes to doCreateAjaxFlag()

When I get something that passes -- the number of tests that should not be converted is good information to have as the parent starts up again.

As always I am flexible - if you disagree - I will adapt the way I work...

> I don't think that is a something we should solve here ... so I am going to restore createFlagWithForm()
and back out the changes to doCreateAjaxFlag()

Well one possibility is that we decide that the various flag link type tests do NOT cover the admin UI, and so don't need JS, and we have a separate test class for the flag admin UI that tests the link type options changing with AJAX.

and we have a separate test class for the flag admin UI that tests the link type options changing with AJAX.

I am with you on that one... I will file a separate issue. for that .. I just want to work in small chunks

As well as the changes from #17 I have a bug fix to describe

creating a flag as follows, falls flat

FlagTestBase::createFlag('user, [] 'reload)

as the flag_type inherited by default from createFlagFromArray() is of type 'node' - the solution ::-

protected function createFlag($entity_type = 'node', array $bundles = [], $link_type = 'reload') {
    return $this->createFlagFromArray([
      'entity_type' => $entity_type,
      'bundles' => $bundles,
      'link_type' => $link_type,
+      'flag_type' => $this->getFlagType($entity_type),
    ]);
  }

bugs always make me smile ... once I see them :)

I have fixed UserFlagTestType

I had to back out my attempt to use

$flag = $this->createFlag('user', [], 'reload');

Previously when we were creating a flag via the form the show_of_profile field was automatically checked.

Our replacement needs to copy that....

So for me this show the limitations of the helper function... it is a good short-cut for bundled nodes only.

silly me.... happy me.

ShowOnEntityFormTest is green

Changes to FlagTestBase

When you create a Flag using the form the show_as_field is enabled by default - no other checkboxes in that area are ticked.

So I have updated the sensible defaults provided by FlagTestBase::createFlagFromArray() to reflect that scenario.

Changes to ShowOnEntityFormTest

The test now correctly creates a flag as if the show_on_form checkbox has been flicked on.

Yup, so am I -- there's an issue to add that back in.