In #1696902: Convert forum_admin_settings() to new configuration system the forum module was upgraded to use Drupal 8's configuration system. The variables forum_block_num_active and forum_block_num_new are updated to the config keys block.active.limit and block.new.limit in forum.install.

At the time we were discussing how config keys should be named this resulted in incorrect config->set's and ->get's for these keys.

At the moment the forum module will set and read the incorrect the keys block.num_active and block.num_new. The functions forum_block_configure() and forum_block_save() need to be updated to use block.active.limit and block.new.limit.

This means that the upgrade path is broken.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
1.07 KB

How's this?

larowlan’s picture

Status: Needs review » Reviewed & tested by the community
sun’s picture

Looks good. :)

Someone will ask for it either way, so do we wanna add tests for forum module's blocks here?

I don't think we need upgrade path tests. Just regular tests for the Forum module blocks should be sufficient. AFAICS, there are none at all yet.

webchick’s picture

Title: Forum module's block configuration for block.active.limit and block.new.limit » Needs tests: Forum module's block configuration for block.active.limit and block.new.limit
Priority: Critical » Normal
Status: Reviewed & tested by the community » Active
Issue tags: +Needs tests

Agreed on the need for Forum block tests, but let's not tie up a thresholds slot with it.

Committed and pushed to 8.x. Thanks!

Moving down to normal task and clarifying intent.

mr.baileys’s picture

Status: Active » Needs review
FileSize
7.8 KB

Here are some tests for the Forum's blocks. ForumTest already contained some really basic tests (basically just enabling the blocks and checking for block title for 'New forum topics' on the page). I moved all 'forum block'-related tests to a new test class, ForumBlockTest.

aspilicious’s picture

Status: Needs review » Needs work
+++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumBlockTest.phpundefined
@@ -0,0 +1,181 @@
+}
\ No newline at end of file

Needs a newline :(

mr.baileys’s picture

Needs a newline :(

No need to be sad about that ;)

New patch attached with a newline, and also removed an excess blank line.

mr.baileys’s picture

Status: Needs work » Needs review

Back to CNR

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumBlockTest.phpundefined
@@ -0,0 +1,180 @@
+    // We expect only the 2 most recent forum topics to appear in the "New forum
+    // topics" block.
+    $this->assertText($topics[3], t('Forum topic found in the "New forum topics" block.'));
+    $this->assertText($topics[4], t('Forum topic found in the "New forum topics" block.'));

I think we also need to assert that the other forum topics are not present. Not just that the two most recents topics are in the block.

+++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumBlockTest.phpundefined
@@ -0,0 +1,180 @@
+    // We expect only the 2 forum topics with most recent comments to appear in
+    // the "Active forum topics" block.
+    $this->assertText($topics[3], t('Forum topic found in the "New forum topics" block.'));
+    $this->assertText($topics[4], t('Forum topic found in the "New forum topics" block.'));

As above. Also this should be "Active forum topics" not "New forum topics".

-enzo-’s picture

I did a reroll for patch in comment #7 using feedback in comment #9

check older forum are not present

-enzo-’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, forum_block_active_limit_test-1775780-10.patch, failed testing.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
0 bytes

Thanks -enzo-

I had also updated my version of the patch, although I used a for-loop instead of individual indices. Both approaches look ok to me.

mr.baileys’s picture

Status: Needs review » Needs work

The last submitted patch, 1775780-12-forum-block-tests.patch, failed testing.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
9 KB

The tests was running fine locally -- I assume the test-failures are caused by forum posts with the same created timestamp being ordered differently, so I added some logic to make sure the timestamps are all different.

Also added assertions to check for the presence of a "more"-link, and changed the assertText with an assertLink for forum titles in the New|Active blocks, since they are links to forum posts.

Tests pass locally, let's see if the bot agrees.

Status: Needs review » Needs work

The last submitted patch, 1775780-16-forum-block-tests.patch, failed testing.

mr.baileys’s picture

Status: Needs work » Needs review

The one remaining test-failure seems unrelated to this patch, so setting to NR for a re-test.

mr.baileys’s picture

#16: 1775780-16-forum-block-tests.patch queued for re-testing.

alexpott’s picture

Status: Needs review » Needs work

These tests rock! I especially like the testActiveForumTopicsBlock test - creating 10 topics and commenting on the first 5... really good work.

+++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumBlockTest.phpundefined
@@ -0,0 +1,207 @@
+      $edit = array(
+        'title' => $title,
+        "body[$langcode][0][value]" => $body,
+        // Forum posts are ordered by timestamp, so force a unique timestamp by
+        // adding the index.
+        'date' => date('c', $timestamp + $index),

Really like this solution... elegant and will prevent random testbot failures - which suck eg. #17, #18... :)

Some really minor nits....

+++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumBlockTest.phpundefined
@@ -0,0 +1,207 @@
+  /**
+   * A user with various administrative privileges.
+   */
+  protected $admin_user;

should be $adminUser - see http://drupal.org/node/608152

+++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumBlockTest.phpundefined
@@ -0,0 +1,207 @@
+  /**
+   * A user with no special privileges.
+   */
+  protected $web_user;
+

not used apart from in setUp. Probably can be removed.

EDIT: minor typo.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
8.87 KB

Thanks for the reviews!

Removed $web_user and renamend $admin_user to $adminUser.

The last submitted patch, 1775780-21-forum-block-tests.patch, failed testing.

mr.baileys’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +Configuration system, +Config novice

#21: 1775780-21-forum-block-tests.patch queued for re-testing.

sun’s picture

heh... I admit this is confusing, but the current variable naming practice in tests is actually $admin_user and $web_user ;) (even when assigning them as object properties)

However, that's only a common practice and not a (bogus) standard in any way, so individual test cases can happily decide on their own. :)

I really like that this patch also cleans up the epic/monster ForumTest case -- which is one of the oldest and first test case classes in Drupal core, and back then, we didn't really know how to properly structure tests, so this thingie is a giant mess of "testing Forum functionality"... ;) Really glad about that. -- However, did we double-check that there are not further parts about the blocks in there?

Lastly, I'm a little concerned about the (seemingly random) test failures -- the last patch came back green, but only after re-testing? Which test failed exactly? If it's a different test, then we can move forward here.

I will send this patch for re-testing a couple of times (administratively on qa.d.o, in order to not spam this issue with re-tests).

Status: Needs review » Needs work

The last submitted patch, 1775780-21-forum-block-tests.patch, failed testing.

sun’s picture

So the random test failures are actually caused by this patch:

Forum topic not found in the "Active forum topics" block.
ForumBlockTest.php 158 Drupal\forum\Tests\ForumBlockTest->testActiveForumTopicsBlock()

Forum topic found in the "Active forum topics" block.
ForumBlockTest.php 155 Drupal\forum\Tests\ForumBlockTest->testActiveForumTopicsBlock()

Re-testing the patch to make it "fake-pass" therefore is not a good idea ;)

Instead, we need to ensure that these random failures cannot happen.

mr.baileys’s picture

Thanks for reviewing sun!

However, did we double-check that there are not further parts about the blocks in there?

The active and new forum topics blocks are also tested in Drupal\forum\Tests\ForumNodeAccessTest, but since the tests there are focussed on node access, I figured they should remain there?

There are some references to the blocks in Drupal\forum\TestsForumTest still, so I'll work on (re-)moving/refactoring those.

Lastly, I'm a little concerned about the (seemingly random) test failures -- the last patch came back green, but only after re-testing? Which test failed exactly? If it's a different test, then we can move forward here.

The instances were I requested a retest where caused by a single test failing in LocaleFileImportStatus, so the re-tests weren't an attempt to force a random green result ;) -- I'm not getting the failures you are seeing on my local machine, but I'm assuming this is because the comments all have the same timestamp which causes the 'active' forum topics ordering to be unreliable. I'll see if I can force a different timestamp on the comments like I did for forum posts, but I don't think this is possible via the interface.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
3.92 KB
11.24 KB

Let's try this one.

  • I did not know about WebTestBase::drupalGetNodeByTitle(), replaced my own query with that function.
  • Cleaned up ForumTest some more
  • Made the comment timestamps unique in an attempt to get rid of the random test-failures.

New patch and interdiff with previous patch attached.

sun’s picture

I wonder whether we shouldn't simply use the API functions for creating those comments instead of the UI/browser? That allows us to specify an exact creation date for each comment entity. And since the test is about the expected block functionality and not about testing the UI for posting comments, using internal APIs to set up test fixtures is actually preferred.

mr.baileys’s picture

Makes sense, re-worked the patch to use comment_save() instead of the interface.

Lars Toomre’s picture

Status: Needs review » Needs work

Thanks for the great work in cleaning up these tests!

One issue that I note throughout this patch is the use of t() around the test assertion messages. This has recently changed and no longer should be used according to http://drupal.org/node/265828. Thus setting back to needs work.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
10.56 KB

Thanks Lars, I've removed all t()'s from assertions.

andypost’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +Configuration system, +Config novice

The last submitted patch, 1775780-32-forum-block-tests.patch, failed testing.

kbasarab’s picture

Status: Needs work » Needs review
FileSize
10.59 KB

Rerolls to current HEAD.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Good to go

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Great work on this. Seems like a nice clean-up of the existing tests, and it's really nice to finally have this follow-up task put away. :)

Committed and pushed to 8.x. Thanks!

sun’s picture

+++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumBlockTest.php
@@ -0,0 +1,197 @@
+  public static $modules = array('taxonomy', 'comment', 'forum', 'node', 'block', 'menu', 'help');

Why does this test enable Menu and Help module?

mr.baileys’s picture

Status: Fixed » Needs review
Issue tags: -Needs tests, -Configuration system, -Config novice
FileSize
607 bytes

Probably an unfortunate left-over from copying another test. The taxonomy, comment and node modules are also unnecessary, since they are listed as dependencies for the forum module.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Bot happy!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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