Move information about tests to a separate module.tests.info (or yml) file

[quicksketch]

Over in #77 (Improve class registry), we've run into an issue where we can't make the class registry more efficient unless we establish that only modules that are enabled should include their classes the registry.

The reason we have this ability to execute classes in modules that aren't enabled comes from SimpleTest, which needs to load each individual .test file in order to get information about each test. Each test includes a static getInfo() method to give the test a name, description, and group. However using a static method like this has multiple downsides:

• It means PHP must actually parse the entire .test file to call the getInfo() methods. That means every .test file in the entire system must be parsed in order to display a list of tests. In Drupal 8 for instance, displaying the list of tests requires over 150MB of memory!
• It means that Backdrop must include all of these classes for disabled modules in the class registry even though they aren't used 99.9% of the time.

To solve this problem, I recommend that we move information about tests to a separate stand-alone files, just like a module's .info file. That will allow us to get a list of tests extremely quickly, with a very small memory footprint (exactly the same thing we do for modules).

Unfortunately, if we want to list multiple tests in the same file, our existing .info syntax is not adequate. However if we convert info files to YAML (which is a good move, IMO), then this problem is solved.

Before change (in book.test file):

class BookTestCase extends DrupalWebTestCase {
  // ...
  public static function getInfo() {
    return array(
      'name' => 'Book functionality',
      'description' => 'Create a book, add pages, and test book interface.',
      'group' => 'Book',
    );
  }
  // ...
}

After change (in book.tests.yml):

BookTestCase:
  name: Book functionality
  description: Create a book, add pages, and test book interface.
  group: Book
AnotherTest:
  name: Other functionality
  description: Other description
  group: Book

[quicksketch]

After reviewing this, it became clear that some modules include multiple .test files. Additionally, these test files can be in different locations, i.e. book.test or tests/webform.test. And some modules have multiple .test files. So we also need to include a file path to point to each test class (not unlike the class registry in #77).

So some alternative formats might be:

book.test
  BookTestCase:
    name: Book functionality
    description: Create a book, add pages, and test book interface.
    group: Book
  AnotherTest:
    name: Other functionality
    description: Other description
    group: Book
tests/yet_another.test
  YetAnotherTest:
    name: Yet more functionality
    description: Yet another description
    group: Book

Or avoiding extra nesting by specifying the file individually:

BookTestCase:
  name: Book functionality
  description: Create a book, add pages, and test book interface.
  group: Book
  file: book.test
AnotherTest:
  name: Other functionality
  description: Other description
  group: Book
  file: book.test
YetAnotherTest:
  name: Yet more functionality
  description: Yet another description
  group: Book
  file: tests/yet_another.test

The second approach is preferred for me, considering there are multiple items that could be repeated (both "file" and "group"). Unless we want deeply nested arrays, repetition is the other option. If we move to a model more like D8, we'd also likely separate individual .test files into multiple .php files, rather than lumping them all together in a single file. That would mean less repetition and more different values, another good reason to go with the latter format.

[jenlampton]

I also prefer the last format.

I love that this solution also allows for the possibility that tests can be run without needing the rest of Drupal (depending on the test, of course) or even the module they come with.

This is a very elegant solution, +1 from me! :)

[xiongxin]

The second +1

[quicksketch]

Thanks guys. I'm working on this issue to unblock the new class autoloader/registry: #77.

[quicksketch]

@larowlan filed a similar issue to match this one for Drupal's loading of tests at https://drupal.org/node/2089681.

[quicksketch]

I've been thinking more about this particular need and I'm not sure if the conversion to YAML can be entirely justified. Alternatively, we could continue using .info files and amend their parsing to support INI-style "sections". Using the same example from above, here's what a module.tests.info file would look like:

[BookTestCase]
name = Book functionality
description = Create a book, add pages, and test book interface.
group = Book
file = book.test

[AnotherTest]
name = Other functionality
description = Other description
group = Book
file = book.test

[YetAnotherTest]
name = Yet more functionality
description = Yet another description
group = Book
file = tests/yet_another.test

This also would make info file format a superset of INI file syntax, supporting all the same syntaxes but adding support for nested arrays. This would be an improvement on info files current situation of a mostly-the-same-but-slightly-different format from INI files. Compared to YAML parsing, it would also likely be faster, though I haven't verified that with numbers yet.

The overall effect of this move would be that Drupal modules would have fewer changes to implement to move to Backdrop. Since that's our first priority, it seems like this approach would be both the fastest and easiest approach to this problem.

[quicksketch]

I filed a PR at backdrop/backdrop#87.

Not surprisingly, moving tests to a dedicated flat-file instead of loading all of the PHP files gets us a significant memory savings:

Before patch:

Execution time: 0.64982604980469, memory usage: 57.64 MB (with APC, first load)
Execution time: 0.41042494773865, memory usage: 37.99 MB (with APC, successive loads)
Execution time: 0.66880106925964, memory usage: 73.65 MB (no APC)

After patch:

Execution time: 0.87765884399414, memory usage: 12.58 MB (with APC)
Execution time: 1.0128488540649, memory usage: 28.83 MB (no APC)

With or without patches, after caching in the database:

Execution time: 0.35862708091736, memory usage: 11.92 MB (with APC)
Execution time: 0.46877598762512, memory usage: 28.15 MB (no APC)

So excluding the "normal" page weight (based on the cached versions), building the list takes this much memory with each approach (with APC disabled):

Old approach: 45.5 MB
New approach: 700 KB

That seems worth the slowdown while loading the page.

I thought this patch would also have residual benefits by removing hundreds of classes from the 'registry' table, but it turns out to have had negligible effect in normal page loading performance.

ab -n 500 -c 10 http://backdrop/

Before patch (no APC):

Requests per second:    16.10 [#/sec] (mean)
Time per request:       620.959 [ms] (mean)

After patch (no APC):

Requests per second:    16.13 [#/sec] (mean)
Time per request:       619.964 [ms] (mean)

---

Before patch (no APC):

Requests per second:    79.13 [#/sec] (mean)
Time per request:       126.381 [ms] (mean)

After patch (with APC):

Requests per second:    78.42 [#/sec] (mean)
Time per request:       127.512 [ms] (mean)

So basically the benefits are entirely in reducing the amount of memory required to list tests. The real benefit in performance will be the replacement of the registry entirely, which is enabled by this patch. See #77 for more info on removing the registry.

[jenlampton]

This all looks really great.

[quicksketch]

I've merged in the PR at backdrop/backdrop#87. We don't have a system for change notices yet, but this definitely requires one (as our first real API change it seems). I've made a new tag for "needs change notice" and tagged this issue as such. I made an issue for creating a system for tracking changelogs at backdrop-ops/backdropcms.org#14 (in the backdropcms.org issue queue).