CommentFileSizeAuthor
#32 2581187-32.flag_.new-admin-ui-test.patch24.2 KBjoachim
#30 2581187-30.flag_.new-admin-ui-test.patch23.49 KBjoachim
#28 2581187-28.flag_.new-admin-ui-test.patch24.47 KBjoachim
#25 2581187-25.flag_.new-admin-ui-test.patch23.58 KBjoachim
#17 2581187-17.flag_.new-admin-ui-test.patch23.5 KBjoachim
#11 2581187.flag_.new-admin-ui-test-11.patch9.63 KBmartin107
#11 interdiff-2581187-3-11.txt828 bytesmartin107
#3 interdiff-2-3.txt3.5 KBmartin107
#3 2581187.flag_.new-admin-ui-test-3.patch9.18 KBmartin107
#2 2581187.flag_.new-admin-ui-test.patch22.68 KBjoachim

Comments

joachim created an issue. See original summary.

joachim’s picture

joachim commented
Status: Active » Needs review
StatusFileSize
new 2581187.flag_.new-admin-ui-test.patch22.68 KB

Big patch, but I've done it as a sequence of commits so that makes it clearer to see what's going on.

Here's the subjects of each commit by way of a summary:

  • Added basic test class.
  • Added doFlagAdd(), based on test method in other classes. Marked other methods that test flag adding for removal.
  • Moved tests for enabling and disabling flags from FlagEnableDisableTest. Marked original methods for removal.
  • Added TODO for flag deletion test.
  • Moved test for resetting flags from FlagResetTest. Marked original methods for removal.
  • Removed FlagEnableDisableTest: all test methods covered elsewhere.
  • Removed FlagResetTest: all test methods covered elsewhere.
  • Removed FlagSimpleTest::doFlagAdd(); replaced with basic flag creation from FlagTestBase.
martin107’s picture

martin107 commented
StatusFileSize
new 2581187.flag_.new-admin-ui-test-3.patch9.18 KB
new interdiff-2-3.txt3.5 KB

@joachim, please forgive me if this is treading on your toes... feel free to ignore this patch

I think your changes are an improvement \Drupal\flag\Tests\AdminUI is the way to go...

before helping out on this I want to make a patch that clears a few things up.

I have cleared what I think is a bug in AdminUI::setup()

    // Create a node to flag.
    $this->node = $this->drupalCreateNode(['type' => $this->nodeType]);
    -$this->node_id = $node->id();
    +$this->node_id = $this->node->id();

$node was undefined when used.

The changes are minor nits only

1) I ran the new test through phpcs and fixed a couple of things.

2) \Drupal\flag\Tests\AdminUI::id - I've changed the name to flagId as I wanted a clear separation from AdminUI::nodeId

3) FlagTestBase makes $this->t() available so I have swapped out uses of t() - which will sometime in the future become deprecated.

running the tests locally fails, but then there are a few todo to resolve.

The last submitted patch, 2: 2581187.flag_.new-admin-ui-test.patch, failed testing.

The last submitted patch, 2: 2581187.flag_.new-admin-ui-test.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 3: 2581187.flag_.new-admin-ui-test-3.patch, failed testing.

The last submitted patch, 3: 2581187.flag_.new-admin-ui-test-3.patch, failed testing.

The last submitted patch, 2: 2581187.flag_.new-admin-ui-test.patch, failed testing.

The last submitted patch, 3: 2581187.flag_.new-admin-ui-test-3.patch, failed testing.

socketwench’s picture

socketwench commented

Same changes as Martin, getting the following error:

Argument 1 passed to Drupal\flag\FlagService::flag() must implement interface Drupal\flag\FlagInterface, null given, called in /var/www/modules/flag/src/Tests/AdminUI.php on line 184 and defined

martin107’s picture

martin107 commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2581187-3-11.txt828 bytes
new 2581187.flag_.new-admin-ui-test-11.patch9.63 KB

Just fixing some nits - nothing to bring the error count down.

Status: Needs review » Needs work

The last submitted patch, 11: 2581187.flag_.new-admin-ui-test-11.patch, failed testing.

The last submitted patch, 11: 2581187.flag_.new-admin-ui-test-11.patch, failed testing.

joachim’s picture

joachim commented

Cool, thanks guys! I'll work on this more tonight.

> 3) FlagTestBase makes $this->t() available so I have swapped out uses of t() - which will sometime in the future become deprecated.

I'm going to leave this one out -- my aim here is for a patch that moves code around as near as possible, so we can see that no test coverage has been lost. Replacing t() calls can be done later.

The last submitted patch, 11: 2581187.flag_.new-admin-ui-test-11.patch, failed testing.

joachim’s picture

joachim commented 
+use Drupal\flag\Tests\FlagTestBase;

I don't think we need that -- we're already in that namespace.

--- a/src/Tests/FlagEnableDisableTest.php
+++ b/src/Tests/FlagEnableDisableTest.php

Don't need to change the import statements here -- we're killing this class.

joachim’s picture

joachim commented
Status: Needs work » Needs review
StatusFileSize
new 2581187-17.flag_.new-admin-ui-test.patch23.5 KB

Status: Needs review » Needs work

The last submitted patch, 17: 2581187-17.flag_.new-admin-ui-test.patch, failed testing.

The last submitted patch, 17: 2581187-17.flag_.new-admin-ui-test.patch, failed testing.

joachim’s picture

joachim commented

> Warning: Missing argument 4 for flag_flag_access() in flag_flag_access() (line 246 of modules/contrib/flag/flag.module).

This seems to be a new problem caused by a change in core.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yes.

http://drupal.org/node/2581447

However, that whole function looks weird anyway. It conflicts with hook_ENTITY_TYPE_access() and is most of the time called with completely different arguments. This only worked accidently because they didn't have directly conflicting type hints.

joachim’s picture

joachim commented

I've filed it here: #2584299: hook_flag_access() clashes with hook_ENTITY_TYPE_access().

> However, that whole function looks weird anyway.

Yup. And none of the flag code invokes it! So all the 'flag only your own node' access code never gets touched.

The last submitted patch, 17: 2581187-17.flag_.new-admin-ui-test.patch, failed testing.

The last submitted patch, 17: 2581187-17.flag_.new-admin-ui-test.patch, failed testing.

joachim’s picture

joachim commented
Status: Needs work » Needs review
StatusFileSize
new 2581187-25.flag_.new-admin-ui-test.patch23.58 KB

Failing tests were due to this in createFlagWithForm():

    // Create an array of defaults.
    $default_edit = [
      'id' => strtolower($this->randomMachineName()),
      'label' => $this->randomString(),
      'flag_short' => $this->randomString(16),
      'flag_long' => $this->randomString(16),
    ];

If you do the submit yourself and don't specify flag_short/flag_long, you get the form's defaults. But if you use createFlagWithForm() and don't specify them, you get createFlagWithForm()'s defaults. I think this is weird! Filed #2585405: createFlagWithForm() shouldn't provide defaults that are different from the actual edit form's defaults.

Status: Needs review » Needs work

The last submitted patch, 25: 2581187-25.flag_.new-admin-ui-test.patch, failed testing.

The last submitted patch, 25: 2581187-25.flag_.new-admin-ui-test.patch, failed testing.

joachim’s picture

joachim commented
Status: Needs work » Needs review
StatusFileSize
new 2581187-28.flag_.new-admin-ui-test.patch24.47 KB

Whoops! Forgot this bit.

(BTW: I've turned off one of the test environments, as getting TWO fail comments was getting a bit tedious. I reckon we only need PHP 5.6 for issue testing. On the other hand, I've enabled PHP 7 and also PostGres for daily branch testing. This new d.org CI is ace!)

Status: Needs review » Needs work

The last submitted patch, 28: 2581187-28.flag_.new-admin-ui-test.patch, failed testing.

joachim’s picture

joachim commented
Status: Needs work » Needs review
StatusFileSize
new 2581187-30.flag_.new-admin-ui-test.patch23.49 KB

Status: Needs review » Needs work

The last submitted patch, 30: 2581187-30.flag_.new-admin-ui-test.patch, failed testing.

joachim’s picture

joachim commented
Status: Needs work » Needs review
StatusFileSize
new 2581187-32.flag_.new-admin-ui-test.patch24.2 KB

Argh! permissions weren't getting set.

joachim’s picture

joachim commented

Ahhhhhh... FINALLY! :D

socketwench’s picture

socketwench commented
Status: Needs review » Reviewed & tested by the community

Looks good.

The last submitted patch, 17: 2581187-17.flag_.new-admin-ui-test.patch, failed testing.

The last submitted patch, 25: 2581187-25.flag_.new-admin-ui-test.patch, failed testing.

The last submitted patch, 28: 2581187-28.flag_.new-admin-ui-test.patch, failed testing.

The last submitted patch, 30: 2581187-30.flag_.new-admin-ui-test.patch, failed testing.

joachim’s picture

joachim commented
Status: Reviewed & tested by the community » Fixed

Thanks for the review!

  • joachim committed 7dc7097 on 8.x-4.x 
    Issue #2581187 by joachim, martin107: Added a test class for all Flag...

Status: Fixed » Closed (fixed)

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