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.
Comment | File | Size | Author |
---|---|---|---|
#39 | 1775780-39-forum-block-tests-modules.patch | 607 bytes | mr.baileys |
#35 | 177580-forum-block-tests-35.patch | 10.59 KB | kbasarab |
#32 | 1775780-32-forum-block-tests.patch | 10.56 KB | mr.baileys |
#30 | 1775780-30-forum-block-tests.patch | 10.5 KB | mr.baileys |
#28 | 1775780-28-forum-block-tests.patch | 11.24 KB | mr.baileys |
Comments
Comment #1
tim.plunkettHow's this?
Comment #2
larowlanComment #3
sunLooks 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.
Comment #4
webchickAgreed 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.
Comment #5
mr.baileysHere 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.
Comment #6
aspilicious CreditAttribution: aspilicious commentedNeeds a newline :(
Comment #7
mr.baileysNo need to be sad about that ;)
New patch attached with a newline, and also removed an excess blank line.
Comment #8
mr.baileysBack to CNR
Comment #9
alexpottI 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.
As above. Also this should be "Active forum topics" not "New forum topics".
Comment #10
-enzo- CreditAttribution: -enzo- commentedI did a reroll for patch in comment #7 using feedback in comment #9
Comment #11
-enzo- CreditAttribution: -enzo- commentedComment #13
mr.baileysThanks -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.
Comment #14
mr.baileysComment #16
mr.baileysThe 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.
Comment #18
mr.baileysThe one remaining test-failure seems unrelated to this patch, so setting to NR for a re-test.
Comment #19
mr.baileys#16: 1775780-16-forum-block-tests.patch queued for re-testing.
Comment #20
alexpottThese tests rock! I especially like the testActiveForumTopicsBlock test - creating 10 topics and commenting on the first 5... really good work.
Really like this solution... elegant and will prevent random testbot failures - which suck eg. #17, #18... :)
Some really minor nits....
should be $adminUser - see http://drupal.org/node/608152
not used apart from in setUp. Probably can be removed.
EDIT: minor typo.
Comment #21
mr.baileysThanks for the reviews!
Removed $web_user and renamend $admin_user to $adminUser.
Comment #23
mr.baileys#21: 1775780-21-forum-block-tests.patch queued for re-testing.
Comment #24
sunheh... 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).
Comment #26
sunSo 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.
Comment #27
mr.baileysThanks for reviewing sun!
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.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.Comment #28
mr.baileysLet's try this one.
New patch and interdiff with previous patch attached.
Comment #29
sunI 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.
Comment #30
mr.baileysMakes sense, re-worked the patch to use
comment_save()
instead of the interface.Comment #31
Lars Toomre CreditAttribution: Lars Toomre commentedThanks 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.
Comment #32
mr.baileysThanks Lars, I've removed all
t()
's from assertions.Comment #33
andypost#32: 1775780-32-forum-block-tests.patch queued for re-testing.
Comment #35
kbasarab CreditAttribution: kbasarab commentedRerolls to current HEAD.
Comment #36
andypostGood to go
Comment #37
webchickGreat 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!
Comment #38
sunWhy does this test enable Menu and Help module?
Comment #39
mr.baileysProbably 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.
Comment #40
andypostBot happy!
Comment #41
webchickCommitted and pushed to 8.x. Thanks!