I'm finding it really hard to work with the tests because, for example, doNodeCreate() doesn't just create a node, but also flags it and checks the flagging and so on.

At some point when the there aren't patches flying around, we need to go through the test classes and rename or split up some of the methods.

CommentFileSizeAuthor
#4 2467013-4.flag_.test-method-refactor.patch4.4 KBsocketwench
#2 2467013-2.flag_.test-method-refactor.patch4.37 KBmartin107
#1 2467013-1.flag_.test-method-refactor.patch4.56 KBjoachim

Comments

joachim’s picture

joachim commented
Status: Active » Needs review
StatusFileSize
new 2467013-1.flag_.test-method-refactor.patch4.56 KB

Found a few other problems with our tests on the way; filed other issues.

martin107’s picture

martin107 commented
StatusFileSize
new 2467013-2.flag_.test-method-refactor.patch4.37 KB

All the changes so far are really good.

This maybe a separate issue to carve off.

Is there a reason we call function of the form doMyTests()
from within testFlagForm() when the convention is that all methods prefixed with test will be executed.

I suspect it is because the order in which the do functions are called is critical, and this is a code smell...

Separately it looks like the following steps are common the majority of the function are could be moved in a setup function for better maintainability.

1) $node = $this->drupalCreateNode(['type' => $this->nodeType]);

2) Give persmiisions 

   // Grant the flag permissions to the authenticated role, so that both
    // users have the same roles and share the render cache.
    $role = Role::load(DRUPAL_AUTHENTICATED_RID);
    $role->grantPermission('flag ' . $this->id);
    $role->grantPermission('unflag ' . $this->id);
    $role->save();

3) create user. and login

Anyway for the moment I am just rerolling, because an issue that was solved here was solved in isolation #2482123: FlagSimpletest - do not continue the process.
.

joachim’s picture

joachim commented

> Is there a reason we call function of the form doMyTests()
from within testFlagForm() when the convention is that all methods prefixed with test will be executed.

Yes, the reason is that a method of the form testFoo() is run in a brand new test setup. So splitting up the test code into testFoo(), testBar() will slow the test down. In many cases it's just that we want to split the code up for clarity, because it tests different parts, but a fresh site isn't necessary. At least, that's my understanding.

> Separately it looks like the following steps are common the majority of the function are could be moved in a setup function for better maintainability.

I think there's an issue open to refactor the tests for more code re-use.

Thanks for the reroll :)

socketwench’s picture

socketwench commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 2467013-4.flag_.test-method-refactor.patch4.4 KB

Needs a reroll, but other than that changes look good to RTBC.

joachim’s picture

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

Thanks!

  • joachim committed d4c02d1 on 8.x-4.x 
    Issue #2467013 by joachim, martin107, socketwench: Changed method names...

Status: Fixed » Closed (fixed)

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