Considering the benefit that we've already gained from better test coverage with the recent additions to project (maintainers, custom issue priorities) and the bugs that have been caught in the 7.x-2.x branch, I'd really like to focus on continuing and expanding our test coverage.

Problem/motivation

To that end, one of the things thats made me get more involved in testing is practicing test driven development (TDD) much more rigorously. To accomplish this, I've been using a tool that watches for filesystem changes and attempts to find the correct module name or test file and run it automatically and use a popup notification on my computer (such as Growl) to respond with the test results. This has made testing much more realtime and much more practical, and I've found it helps to have some specific organization of test files to help with this.

While I don't want to hi-jack the structure of our tests, I do think some better organization would help them, while having very few if any downsides.

Proposal

Organize our tests so that they match the following guidelines:

  1. Only 1 class per file, create as many files as necessary for separating test functionality
  2. Only 1 test method per class
  3. Separate test files into subdirectories inside the tests/ directory
    1. integration/ for tests using the simpletest browser. All of these inherit from DrupalWebTestCase
    2. unit/ for strict unit tests only using DrupalUnitTestCase
    3. spec/ for tests that use the database but do not use the browser. Will inherit from DrupalWebTestCase even though the browser is not used in order to gain access to database functions. Also, I'm up for a better name for this, but spec is the best thing I can think of.
  4. Allow a top level project(_issue).test in the tests/ directory for shared code (i.e. common helpers assertions). Individual classes in unit/ spec/ integration/ subdirectories would inherit from the classes in this top level file. This file should not contain any test methods itself.
  5. The group name should be the titlized version of the short name of the module whenever possible. I'd prefer this to be an absolute rule, but this allows a feature, such that detecting the module name given the filepath of a given changed file. i.e. "Project" "Project issue" or "Project Usage" etc...

Pros:Makes organizing and finding test easier, enables test driven development, should not affect performance of tests, and tests shouldn't affect performance of module, despite the number of files.

Cons:Lots of extra files, repeated boilerplate (i.e. getInfo() or setUp()), many more items listed in the testing UI

I'm looking for feedback or improvements/changes to this proposal, and would like to start implementing something like this soon (and implement it in Project Issue as well).

Comments

mikey_p’s picture

Issue tags: +project, +drupal.org D7

Tagging

dww’s picture

Status: Active » Needs review

That all sounds great to me. The con seems totally dwarfed by the pros.

However, I'm not sure I agree with your assertion "should not affect performance of tests" since I'm pretty sure simpletest installs and destroys an entire Drupal instance for every distinct test class. I believe that having multiple methods per class when you can run multiple tests on the same site install is a huge performance win.

I'd need to see benchmarks of running the full test suite before/after this kind of restructuring to ensure there's not a major performance degradation.

But if you're right that it's no slower, this seems like a best practice that should be discussed and documented in a wider circle than just Project*.

Thanks for writing this up!
-Derek

mikey_p’s picture

Regarding performance, I asked about this in #drupal-contribute and was told that currently the setUp and tearDown methods are called and Drupal is installed once per test method. There is some talk about changing that in D8 (see #1411074: Add a flag to set up test environment only once per test class; introduce setUpBeforeClass() and tearDownAfterClass() [PHPUnit]), but at least for now, it doesn't look like this will change in D7.

dww’s picture

Assigned: Unassigned » mikey_p
Status: Needs review » Reviewed & tested by the community

Ahh, okay. So it's no degradation to split things up. It's already as bad as it can get. ;) Then I say, plow ahead and see how it goes...

Thanks!
-Derek

dww’s picture

@mikey_p: Is this still on your radar/plate? Should we unassign this and see if anyone else wants to work on it? Should I do anything to help this along?

Thanks!
-Derek

drumm’s picture

Assigned: mikey_p » Unassigned
Issue tags: -project, -drupal.org D7

For Drupal.org D7, we will rely on BDD and manual testing.