Problem

  1. The getInfo() method in test classes is a total alien in PHPUnit tests. (Drupalism)

  2. The getInfo() method duplicates meta data that is required by the documentation gate already:

    /**
     * Tests for filter hook invocation.
     */
    class FilterHooksTestCase extends DrupalWebTestCase {
      public static function getInfo() {
        return array(
          'name' => 'Filter format hooks',
          'description' => 'Test hooks for text formats insert/update/delete.',
          'group' => 'Filter',
        );
      }
    

    → The 'description' duplicates the phpDoc summary.

  3. The 'name' duplicates the 'description' and the class name.

  4. Many test classes don't have any phpDoc at all.

  5. → Too much useless, custom, and cumbersome meta data.

Goal

  1. Stop this nonsense and introduce a sane + powerful DX.

  2. Fully align Simpletest with PHPUnit.

Proposed solution

  1. Use phpDoc and only phpDoc to describe tests. Enforce phpDoc for test classes.

    /**
     * Tests hooks for text formats insert/update/delete.
     *
     * @group Filter
     */
    class FilterHooksTestCase extends DrupalWebTestCase {
    }
    
  2. Leverage phpDoc meta data for test code coverage analysis in PHPUnit tests, instead of repeating the obvious:

    /**
     * @coversDefaultClass \Drupal\views_ui\ViewListBuilder
     * @group Views
     */
    class ViewListBuilderTest extends UnitTestCase {
    }
    

Questions and Answers

Can you elaborate on your plan to use test class names and annotations in the Simpletest UI?

I want to remove the getInfo() method entirely.

So as to remove that Drupalism from PHPUnit tests, and to bring our Simpletest tests more in line with PHPUnit/Behat. A test will only have a class name, a description (phpDoc summary), and @group(s).

But most of the existing class names are very undescriptive? They don't really identify what exactly is being tested?

The description (phpDoc summary) will still be exposed in the Simpletest UI, so you can still instantfilter/search for keywords.

Our test "names" are duplicating the description and the class name. We simply have too much useless meta data.

Furthermore, PIFR + qa.drupal.org test results from testbots no longer expose the test "name" for 2+ years already. No one complained. Everyone got used it.

So the description will be an annotation like @group? How will the information be derived from a test class name?

The info will not really be "derived", it will be parsed and taken over as-is.

It will simply become logical and common sense for test authors, and it will enforce good coding standards at the same time, because native phpDoc will replace getInfo():

  1. The test name/description will be the phpDoc summary line of the test class.
  2. The group will become @group.
  3. The test name will become the class name; the current 'name' property will be dropped altogether.

Maybe you have a point. We probably don't need a human-readable title and a human-readable description.

Exactly. :-) Especially because I'm copy/pasting the same string into both locations in 100% of all cases right now.

The following encompasses all meta data that is required for a test:

/**
 * Tests rendering of tables.
 *
 * @group Render
 */
class RenderTableTest {
}
This reminds of plugin annotations [...]

Yes and no.

Yes, the active usage of phpDoc and annotations is definitely not news in D8.

But no, we will not use the annotation parser for tests, because that's overkill. Instead, we leverage 90% of PHPUnit's annotation parsing code, which is completely sufficient for the task at hand and which thankfully lives in static utility classes.

Doing so also sets the right vision and direction: s/custom/phpunit/g

Why do we need this meta data anyway? — PHPUnit does not...?

The testing framework can be broken down into the following:

  1. Discovery (+ optionally filtering) of (all) available tests
  2. A user interface to list and select tests to run. (UI = CLI|UI)
  3. Running one or more tests.

PHPUnit does not have a facility to list available tests. It doesn't need one, because even when running all available tests, the job is expected to complete in almost no time. Therefore, PHPUnit skips (2) and just simply (1) discovers + (3) executes.

However, a single web test easily takes more time to run than the whole PHPUnit test suite. The full test suite takes entire hours to run, even on a reasonably sized machine. Therefore, you want to list + select first. A selection requires a UI.

Why do we need documentation anyway? — …would be the next best question in line. We definitely do want to document tests.

But instead of duplicating information in multiple cumbersome ways, we can leverage documentation in a sensible way.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Version: 7.x-dev » 8.x-dev

No, we're not doing any more of this anti-pattern of putting machine-parsed descriptions in PHPDoc on my watch. The update system is bad enough. Furthermore, this is a huge API change for absolutely zero gain. You can try it with the 8.x maintainer if you want.

Dave Reid’s picture

-1. I don't feel like we really get much benefit for this considering the complexity this would add.

jhodgdon’s picture

I'm also not really in favor of having PHP parse doxygen comments as part of the testing system.

sun’s picture

Issue tags: +Testing system

This was originally proposed in #376129: Change getInfo() to a static method already, came up in the discussion about discovery of test cases in #1477218: Convert Tracker tests to PSR-0 / PSR-0 test class discovery again, and is also in line with one of the discovery mechanisms being discussed for the unified plugin system. It's about time to get it done.

sun’s picture

Title: Remove 'description' from getInfo() and use PHPDoc summary instead » Remove 'name' from test class getInfo() and use PHPDoc summary as 'description' instead

Adjusting issue title for the goal of this issue, which became slightly revised in the meantime.

If we additionally find a way to automate/drop the 'group', then getInfo() can be omitted entirely for most tests.

Crell’s picture

Annotations, perhaps? We have the library available now, and what we're talking about is just metadata, so...

sun’s picture

Yeah, I thought about a @group or @TestGroup annotation, too.

OTOH, a possibly much larger issue is, the current groups.... erm, what, exactly, are they helpful for?

The only purpose they currently have is to "group" tests by... extension.

That can be easily achieved in an automated way though. We can automatically group by extension, or even by namespace, or even both.

My impression is that we have way too much metadata for tests, and for absolutely no good reason. Apparently, the d.o testbot fails to expose all of this... and, wow, we survived! ;-)

I think what I'm ultimately after is that test authors should be able to do this:

namespace Drupal\icecream\Tests;

/**
 * Tests chocolate chips on strawberry-sauced ice-cream.
 */
class ChocolateSauceIceCreamTest extends IceCreamTestBase {

  function testBasicStrawberries() {
    $this->getIceCream();
    $this->addMilk();
    $this->assertTaste('chocolate');
    $this->assertTaste('strawberry');
  }

}

...and be done with it. In other words:

- PHP code is described where it should be described - in phpDoc.

- No need for a name, the class name is sufficient.

- No need for a group, it's going to be "icecream" either way.

Crell’s picture

Well, it's not *quite* that simple. Consider that system module is still fronting for all Component and Core tests, which means that the "system" derived-group is going to be something like 1/4 of the tests in the system, rendering it fairly useless. There's probably a way around that, but it's an issue that we'd need to deal with.

Perhaps sane derived defaults (based on namespace, docblock, etc.), with the ability to override via annotation? That would let us keep the database tests, router tests, menu system tests, etc. in separate groups with an annotation override, but for modules (core or contrib) they 90% of the time don't need to care. But they could if they really wanted to.

And I hereby require that the sample test in #7 make it into core somehow. :-)

tim.plunkett’s picture

After completely reorganizing the Views tests, I can say that all of our groups are mapped almost 100% to the namespace.

I will say that the group is helpful when locally running tests through the UI.

sun’s picture

@Crell:
I played with the idea of potentially solving this through two automated ways:

1) For stuff like System and Views tests, consider each subdirectory within the extension's \Tests namespace as an own [sub-]group. Design the group functionality in a way that allows to run tests of the entire group including sub-groups, but also individual sub-groups only. Sounds natural and straight-forward to implement and use to me.

2) The only other case I can see are plugins-alike tests, for which a developer might want to run all plugin-type-specific tests across the board at once. For this, I think we'd rather have to implant a new "tags" feature for tests though. And tags could be helpful in a much more modular and multi-dimensional way — e.g., to run all "field" related tests, or "mail" related tests, or "session" related tests, etc.pp. Quite some potential there.

In any case, both of these appear doable to me, and would be far superior to the current 'group' definition.

I also want to amend early that I already looked into PHPUnit as well as other PHP applications/competitors that are using PHPUnit to check for whether they have any concept like this, and they do not.

The topic of application-integration tests in a modular/extensible framework system is essentially why the migration and our future usage of PHPUnit will have to diverge from pure PHPUnit, at minimum for non-unit tests — and once we have a solid solution (which I'm sure the Drupal community will be able to come up with), we'll be able to contribute that back upstream to PHPUnit itself - which in turn will be helpful for a ton of other PHP framework/application projects, as it's relatively clear that all of them are struggling with the fundamental question of how to design and run tests in a modular/extensible system. (That said, all work on integrating with PHPUnit is scheduled and postponed to after feature freeze.)

@tim.plunkett: Do you have a more specific use-case for running tests through the UI that is different from running tests per extension/namespace?

Also, please note that #1683884: Update, simplify, and speedup run-tests.sh for PSR-0, and allow to run all tests of a module introduced a new --module argument to run-tests.sh, which allows to run all tests of a module. Not sure whether that fully works already, but at least, it's kind of a precursor to this topic.

tim.plunkett’s picture

The concept of tags for tests sounds appealing to me. Especially if its like GMails tags vs Outlooks folders :)

@sun, no specific use case, other than running all Views UI tests at once, for example.

Crell’s picture

Another data point: Symfony uses PHPUnit, and relies on annotations of some form on individual test methods to handle test dependencies. I don't fully grok it myself, and I'm not necessarily advocating for that at the moment, just noting that annotations-in-tests does have some precedent and is worth our time to look at in other systems.

sun’s picture

@Crell: Yeah, I'm fairly sure that you're referring to intra test method dependencies, which is a built-in feature of PHPUnit (denoting that a test method may only run if another has run), but that's unrelated here, since the metadata we're talking about is per-class. PHPUnit generally makes use of many annotations for test methods (in a good way). Anyway, the only concept it has to bundle/group tests is a phpunit.xml file to specify so called test suites. These are hard-coded though, and thus don't really work for a modular/extensible system (as I alluded to in #10 already). In general, no worries, I'm already on the quest to slowly slice and dice Simpletest in a way to pave the way to a later migration to PHPUnit, so I'm actively looking in how stuff works in there and how others are solving (or not solving and struggling with) certain things. ;)

sun’s picture

Assigned: Unassigned » sun
Issue summary: View changes
Status: Active » Needs review
Issue tags: +DX (Developer Experience), +PHPUnit
Related issues: +#1667822: Remove caching of test classes in TestDiscovery
FileSize
19.2 KB

Sorry, this patch is big, and does not even accomplish the goal (yet), because

  1. I'm merging #1667822: Remove caching of test classes in TestDiscovery into this issue
  2. Profiling the /admin/config/development/testing page revealed plenty of nasty surprises.

All changes related to 2) will be split out into separate issues (and/or reverted), but for now, I had to get a clean picture of what exactly causes that page to take 25 seconds to get rendered (92% of the time is spent in drupal_render()).

For the actual primary change here, I was playing with the idea of copy/pasting the code from Drupal\Component\Annotation\Plugin\Discovery\AnnotatedClassDiscovery — i.e.:

  1. Leverage the StaticReflectionParser (which parses the first class docblock in a file only).
  2. Only use the annotation facility for @group.
  3. Use the class name as test "name".
    → Originally, it was a bug in PIFR after the PSR-0 conversion, which caused testbots to no longer show "names", but only classes. Meanwhile, everyone got used to classes.
    → Drop that "name", without replacement.
  4. Use the phpDoc summary as description.
  5. Kill getInfo().

I will definitely test + bench the StaticReflectionParser approach, but as a very concrete alternative to that, it would also be easily possible to (re-)implement a simplified + use-case-specific docblock parser here...

— That said, please note that the switch to registering a classmap instead of test namespaces significantly improved performance already; i.e., those 1,000+ calls to getInfo() methods pretty much vanished from my profiling results, since the responsible code in ClassLoader boils down to isset($this->classMap[$class]) with that.

But yeah, nothing of the outlined plan is implemented yet, as I got stuck on the overall, peripheral rendering performance of the UI test listing page...


Changelog (against HEAD):

  1. Added TestDiscovery service.
  2. Fixed #2238725: ConfigLanguageOverrideTest is not in ConfigLanguageOverrideTest.php; Provide Memory/Null cache backend factories to facilitate benchmarks.
  3. Do not double-register namespaces of already enabled modules.
  4. Use a class map instead of registering individual test namespaces, since filesystem scan has to happen anyway.
  5. Minor performance improvements for various code in render pipeline.
  6. Fixed cache tag + post render cache in drupal_render() causes every recursion to *additionally* recurse 2x into children of each recursion level.
  7. Fixed unnecessary usage of cufa() in PHP 5.4.
  8. Statically cache the expensive classmap (to combat potential container rebuilds).

Status: Needs review » Needs work

The last submitted patch, 14: test.discovery.14.patch, failed testing.

sun’s picture

sun’s picture

#2262483: Some PHPUnit tests do not exist

...only reinforces me to complete this effort. ;) — Made some very substantial progress, hope to be able to post a patch very soon.

Xano’s picture

What if we just drop the description altogether and only go with @group? This will save one t() call per test class and less table cells means less performance loss in drupal_render(). I am suggesting this, because run-tests.sh and the testbot work with test class names only and don't use the human-readable names or descriptions at all, and just I don't think I've ever seen anyone run a test because the description made it look like it would test the code that was just changed. Most people either know which tests to run, or let the testbot run everything to find out which ones fail, which means they still know which specific tests to run themselves.

Xano’s picture

Status: Needs work » Needs review
FileSize
2.09 KB

This is just to illustrate my idea. It feels slightly faster, though I did not perform any proper benchmarks.

Status: Needs review » Needs work

The last submitted patch, 19: drupal_697760_19.patch, failed testing.

sun’s picture

Alright, I've completed the new test discovery implementation. Just waiting for aforementioned trivial patches to get out of the way, because they make the test discovery changes borderline impossible to review, and without them, the test runner (luckily) explodes with exceptions. :-)

While waiting for that to happen, I'll move the off-topic performance optimizations from #14 into a new issue, and I'm also going to rewrite the issue summary to reflect the actual proposal.

sun’s picture

Title: Remove 'name' from test class getInfo() and use PHPDoc summary as 'description' instead » Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit)
Priority: Normal » Major
Issue summary: View changes
Status: Needs work » Needs review
FileSize
26.75 KB

Here we go!

Updating issue title + Significantly rewrote the issue summary to properly reflect the proposal (which was part of this issue since the beginning, but not really clear).


Changelog:

  1. Implemented new test discovery.
  2. Updated for PSR-4 classloader.
  3. Adjusted UI test listings for new test class information.
  4. Removed obsolete TestBase::getInfo() that enforces the method to be implemented.
  5. Added example for new test class phpDoc + annotations in Action ConfigurationTest.
  6. Polished cowboy stuff in run-test.sh.

Status: Needs review » Needs work

The last submitted patch, 22: test.discovery.22.patch, failed testing.

Berdir’s picture

  1. +++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestTestForm.php
    @@ -192,14 +188,14 @@ public function buildForm(array $form, array &$form_state) {
         foreach ($form_state['values']['tests'] as $class_name => $value) {
           // Since class_exists() will likely trigger an autoload lookup,
           // we do the fast check first.
           if ($value === $class_name && class_exists($class_name)) {
    -        $test_type = in_array($class_name, $phpunit_all) ? 'UnitTest' : 'WebTest';
    +        $test_type = isset($phpunit_all[$class_name]) ? 'UnitTest' : 'WebTest';
             $tests_list[$test_type][] = $class_name;
           }
         }
    

    Unrelated, but I guess an array_filter() on the values would save the first check and lengthy comment?

  2. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestDiscovery.php
    @@ -0,0 +1,396 @@
    +
    +    // Check that each class has a getInfo() method and store the information
    +    // in an array keyed with the group specified in the test information.
    +    $groups = array();
    +    foreach ($classes as $classname => $pathname) {
    

    Outdated comment I think?

  3. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestDiscovery.php
    @@ -0,0 +1,396 @@
    +   */
    +  public static function parseTestClassSummary(\ReflectionClass $class) {
    +    $phpDoc = $class->getDocComment();
    

    Hm, I think the doctrine annotation parsing avoids using reflection to avoid loading all the classes into memory, but that in turn makes it slower.

  4. Can we discuss about the removal of the cache in a separate issue and keep it here for now? I think the cache is more useful than not and with our amount of tests, it's getting *really* slow to parse them all, and 256MB memory is not enough anymore for me to parse and display them. Haven't tried yet how this affects it, but I doubt it will make it faster.
sun’s picture

Status: Needs work » Needs review
FileSize
28.24 KB
18.55 KB
  1. Updated for #2247287: Drop automatic PSR-0 support for modules
  2. Removed obsolete comment about getInfo().
  3. Fixed #2262483: Some PHPUnit tests do not exist *once more*.
  4. Reverted misguided classmap changes + properly split TestDiscovery methods.

Not going to add back persistent caching for now, because (1) there's still a static/property cache, (2) since static::getInfo() is invoked in HEAD, the total required memory is identical to HEAD, and thus (3) performance concerns may only be raised against the runtime performance, but I'm testing on the Worst Case Scenario™ myself (Windows, NTFS, …)

tim.plunkett’s picture

+++ b/core/modules/action/src/Tests/ConfigurationTest.php
@@ -11,7 +11,10 @@
+ * @requires module action

@@ -22,14 +25,6 @@ class ConfigurationTest extends WebTestBase {
   public static $modules = array('action');

How are these not duplicating each other? I don't see anything about adding @requires in this issue so far. Probably should just leave that out.

Status: Needs review » Needs work

The last submitted patch, 25: test.discovery.25.patch, failed testing.

sun’s picture

@tim.plunkett:

@requires module foo is the equivalent of MyTest::getInfo()['dependencies'].

The 'dependencies' key of getInfo() appears to be missing in all documentation & handbook pages.

It specifies "requirements", a list of (module) extensions, which need to exist in order for a test to appear; i.e., before the test runner attempts to instantiate the test class.

public static $modules specifies a list of modules that should be installed (WebTestBase) or loaded (KernelTestBase) for each test method of test class. It presumes that the modules are available.

The point of this issue is to get rid of the getInfo() method. Luckily, PHPUnit already implements a pattern of @requires <type> <name>. This patch simply advances on that.

tim.plunkett’s picture

Yes, but ConfigurationTest::getInfo didn't use the dependencies key, so why would you add it in this patch?

The last submitted patch, 25: test.discovery.25.patch, failed testing.

The last submitted patch, 25: test.discovery.25.patch, failed testing.

Xano’s picture

@jthorson confirmed that the dependencies key predates *.info.yml. Because of that, I am wondering if we shouldn't drop support for dependencies/@requires, because developers should make sure test dependencies are always available anyway.

sun’s picture

Status: Needs work » Needs review
FileSize
28.84 KB
997 bytes
  1. Clarified action/ConfigurationTest with a temporary @todo.
  2. Fixed #2278117: Bad namespace import in Drupal\aggregator\ItemStorageInterface

@Xano:

test_dependencies in .info.yml files serve a different purpose: They signal to the PIFR infrastructure what (Drupal.org) dependencies should be additionally downloaded / checked out when provisioning the test environment.

Given that none of this is clearly documented anywhere, I can totally understand the confusion though. Here's a quick summary:

  1. test_dependencies in .info.yml: Specifies which drupal.org projects should be downloaded when provisioning the test environment.

    Effect: The specified projects are available in the filesystem.

    Managed by: https://drupal.org/project/project_dependency

  2. 'dependencies' in getInfo(): Supplies metadata for a test class to signal which (module) extensions MUST be available in order to include the test class in the discovery. If requirements are not met, the test class is excluded entirely and not visible.

    Effect: If you're trying to run tests for contrib module X that optionally supports module Y, but you do not have module Y locally, the relevant tests are not exposed in the/any UI listing and not run.

    Managed by: Core test discovery.

  3. public static $modules in a test class: Specifies which modules should be installed (WebTestBase) or loaded (KernelTestBase) by default for each test method in the class.

    Effect: You've just been lazy. You could have manually called ModuleHandler::install() (WT) or $this->enableModules() (KTB) yourself. This test class property just automates exactly that, no more, no less.

    Managed by: WebTestBase::setUp() | KernelTestBase::setUp()

In short: Provisioning != Requirements != Installing/Loading. These are three very discrete cases.

Status: Needs review » Needs work

The last submitted patch, 33: test.discovery.33.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
32.14 KB
10.16 KB
  1. Fixed #2278453: Various test getInfo() methods are using t()
  2. Fixed #2278499: Drupal\custom_block\Tests\Menu\CustomBlockLocalTasksTest fails in HEAD
  3. Fixed Simpletest self-tests asserting getInfo() 'name's instead of class names.
  4. Fixed classloader manually looks up each reflected class.

Status: Needs review » Needs work

The last submitted patch, 35: test.discovery.35.patch, failed testing.

Berdir’s picture

Viewing the testing overview:

HEAD no cache: 6.8s / 195M (236M peak)
HEAD cache: 3.7s / 22M (60M peak)

This patch: 4.2s / 193M (240M peak)

So it's not much slower but the memory usage is as big as on HEAD and will grow when you add contrib modules (not bigger than HEAD with an empty cache, obviously, but still...)

What's also interesting is that according to xhprof, 40% of the time spent is in Element::children(), which is called 40k times, and most of those calls are various render cache collection functions. and 60% is in drupal_render() (for the patch, on cached HEAD, it's 67% in the one run that I looked at). Looks like most of those functions call Element::children() 4k times. The reason for that seems to be that template_preprocess_table() calls drupal_render() on every table cell if it's an array, so that sums pretty fast on that huge table (4,377 calls to drupal_render() in that function alone, and 5k+ new Attribute(), also crazy.

So, I still think that keeping the cache would be preferable because waiting a long time on that page is annoying. In fact, if we keep the cache, then it should be very easy to actually use the render cache too which should get us to 1-2s?

Wim Leers’s picture

I don't have the time to dive into this patch right now, but AFAICT this is about caching test metadata, right? And one of the remaining performance problems is the fact that rendering a huge table like this is super expensive due to inefficiencies in table rendering and cache tag collection, right?
Why don't we just render cache the entire tests table? :)

sun’s picture

re: @Berdir: Crazy page rendering profiling results:

Yep, that massive render/theme processing is what totally distracted me earlier already; see #14.

I moved some smaller fixes into #2263279: Replace call_user_func_array() with $callback() for performance, where possible.

I helped to complete #1939008: Convert theme_table() to Twig, which was one of the last blockers for refactoring the table rendering code. The remaining blockers are to convert all #theme table into #type table. Once that is done, we can finally optimize the entire table processing code, since we no longer need to support a second, alternative call chain.

Additionally, I'm very confident that there are multiple recursion bugs in the current code of drupal_render(). That's why I created #2239457: Move main complexity of drupal_render() into Drupal\Core\Render\Render. Once the code is moved, we can properly unit-test the expected call chains within the Render class methods themselves.

Both the table processing performance as well as the drupal_render() recursion performance are independent problems of their own — the test listing page is just a symptom; it just happens to expose/reveal the problems under the hood. Any other page/table of a similar size will run into the same problem. Not going to be fixed here.


re: @Berdir/@Wim Leers: Actual/relevant performance & caching:

Thanks for doing the HEAD/patch CPU/memory performance comparison! That's in line with my local numbers thus far + matches my expectations in a very positive way. I wasn't fully done with deeper profiling yet, so further optimizations may be possible.

As outlined in #1667822: Remove caching of test classes in TestDiscovery, caching the list of test classes is 100% counter-productive and works against you as a developer and against the primary purpose of the test discovery/listing UI. Whenever you add a new test class, and whenever you rename a test class, and whenever you delete a test class, and whenever you're switching branches, you have to manually flush the cached list first.

This is a very concrete case in which D8's new mantra of "Slap a Cache on Everything." clearly fails to meet the requirements of the affected functionality.

This code was explicitly designed to make the cache superfluous. We need to handle the cold cache scenario anyway. Lastly, the new code re-uses some code from PHPUnit; improvements may be possible upstream. (I do wish we could re-use more of it.)

However, if the caching topic keeps on coming up, then the maximum compromise I could live with would be a cache item that not only stores the discovered test classes, but additionally dir => filectime + file => filectime mappings — when retrieving the list from cache, all entries would be compared against the current filectime(), and if there's any difference, the cache is ignored.

sun’s picture

Status: Needs work » Needs review
FileSize
27.75 KB
849 bytes

Reverted additional code cleanup/fix to be tackled/committed in #2278499: Drupal\custom_block\Tests\Menu\CustomBlockLocalTasksTest fails in HEAD


The BrokenSetUpTest failure appears to be constant and not random. But I'm not able to reproduce it locally.

It does not occur in HEAD. It seemingly appears to tilt at a limit of 256MB. Which could be caused by stacked up PHP child processes, but then again, the child-child-test-site of this test runs in a separate webserver process, since this is a web test...

I'll try to futz with my local PHP ini settings, but meanwhile, I wonder whether anyone else is able to reproduce the failure locally?

Status: Needs review » Needs work

The last submitted patch, 40: test.discovery.40.patch, failed testing.

The last submitted patch, 40: test.discovery.40.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
35.68 KB
11 KB
  1. Removed all remnants of getInfo() within test runner.
  2. Removed obsolete simpletest_phpunit_get_available_tests().
  3. Do not re-run test discovery when submitting the UI form with an explicit list of test classes.

Status: Needs review » Needs work

The last submitted patch, 43: test.discovery.43.patch, failed testing.

Wim Leers’s picture

#39: I understand, and agree. Especially because it sounds like the slow-by-design rendering of tables will go away.

But… we're not there yet. IDK what the timeframe is for fast table rendering? So I think your proposed compromise makes a lot of sense. I'm assuming you're referring to a two-tiered approach: there's a cache item containing all the directory & file ctimes, which is checked every time the "testing" page is loaded, and if still valid, then the render cache item is loaded.
I think/hope that would result in a massive speed-up.

Also: thank you very much for the crystal clear explanation — I appreciate it! :)

sun’s picture

Repeating only BokenSetUpTest 20x times: https://qa.drupal.org/pifr/test/800343

Weird results: The first 4 attempts failed, the remaining 16 passed. Not sure how that is possible.

moshe weitzman’s picture

+1 for this patch. We are quickly getting in-line with PHPUnit approaches which is a very good thing.

chx’s picture

the changes in CustomBlockLocalTasksTest imo doesn't belong here

tim.plunkett’s picture

In a couple issues, #2262483-20: Some PHPUnit tests do not exist being the most recent, the claim has been made that this issue would somehow prevent tests from not getting picked up.

Can you expand how this would help if the annotations were left off, or if the file name doesn't match the class name?

For example, until yesterday, the file IgnoreReplicaSubscriberTest.php had class IgnoreSecondarySubscriberTest

dawehner’s picture

This issue solves the cases which forgot to use getInfo() or use the phpunit base class. This itself is a huge improvement, obviously.

Can you expand how this would help if the annotations were left off, or if the file name doesn't match the class name?

I think everyone knows how to solve that properly: Don't run phpunit tests through simpletest anylonger but use phpunit which uses its own autoloading so you never run into that problem in the first place. Yes it is not perfect if you don't match the names but this is really just a PSR0/4 problem, not phpunit.

chx’s picture

As for picking up, having a getInfo() is easily discoverable, visible as every test has it so no matter what you look at as an example, it's there. The same can be said for the annotation, of course.

But if you want easy discovery then nuke the *Test.php requirement because it's implicit, subtle and impossible to learn by example.

Edit: that's obviously a followup.

catch’s picture

Issue tags: +beta deadline
sun’s picture

Status: Needs work » Needs review
FileSize
38.91 KB
1.26 MB

Sorry for the silence.

I've been concerned about the excessive memory consumption of discovering tests (with just core) and spent the last weeks with excessive profiling/benchmarking. Found a solution that resolves the problem (reducing memory consumption to ~10MB with a peak of ~32MB). — However, others asked me to NOT include that solution here, because the problem exists in HEAD already, and because it is important to get the primary API change into core before the beta release. Therefore, the memory consumption can and will be addressed in a separate follow-up issue.


Due to that, the implementation in attached patch is not substantially different than previous patches. I focused on the following tasks:

  1. Fix trivial/home-grown performance issues (e.g., scanning directories that do not contain tests).
  2. Come up with a script that is able to reliably parse + rewrite arbitrary PHPDoc of 1,300+ classes.
  3. Retain getInfo() as a temporary BC layer until after the beta release, so as to prevent other beta deadline patches from breaking HEAD.

The conversion script uses the following logic:

  1. Retrieve the current getInfo(), and retrieve + parse the current PHPDoc of the class.
  2. Replace the PHPDoc summary line with the 'description', unless the summary line is very similar, in which case the summary is almost always more accurate.
  3. For PHPUnit tests, remove the PHPDoc summary line in favor of @coversDefaultClass, if applicable.
  4. Determine the @group based on the class's namespace, resulting in the originating Component name or module name.
  5. Retain a possibly existing long description as well as other PHPDoc tags/annotations.

(see also #2057905-47: [policy, no patch] Discuss the standards for phpunit based tests)

The conversion script performs a "hard reset" of all @group annotations to be either the component name or the module name. Secondary groups are removed. IMO, that is fine/desired, because the hard reset establishes consistent @group names for all test classes. The standardization enables you to run all tests of a module or component without having to look up the group name first - e.g., running the 'node' group will run all tests of the Node module extension, while 'Routing' will run all tests of the Routing component. Consistent and simple.

That said, we should have a dedicated discussion about appropriate secondary/additional @group names, starting by determining sensible use-cases; see #2297541: [policy, no patch] Secondary/additional test @group names

I personally consider attached patch to be mostly complete, so reviews are appreciated.

sun’s picture

FileSize
39.92 KB
4.5 KB

A few clean-ups:

  1. Restored test info cache (applies to UI only); removed superfluous property cache.
  2. Removed duplicate sorting of tests from UI form.
  3. Clarified revelation of namespace/classname/pathname mismatches.

Will only attach a new full conversion patch in case of changes (or when entering RTBC status).

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

IMO, this is RTBC. I guess Sun should attach a full conversion and then a core committer will give final review?

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs review

This needs a draft change notice, doesn't it, before it's ready for commit?

sun’s picture

Change notice: https://www.drupal.org/node/2301125


Final adjustment:

  1. Ensure that legacy tests appear in the same groups as already converted tests.

To clarify: After pulling latest HEAD, already converted tests appeared in different groups than newly introduced (legacy) tests. The BC layer for getInfo() in TestDiscovery uses the identical algorithm for determining the (primary) @group as the conversion script now (see interdiff).

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the change notice. I personally don't need all the background motivation in a change notice. Just the facts, and a link to learn more. Meanwhile, this is RTBC again.

xjm’s picture

Yeah, I'd suggest this change to the CR:
https://www.drupal.org/node/2301125/revisions/view/7427169/7428201

Great to see this RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, great. Getting this in while it's hot!

Committed and pushed to 8.x. Thanks!

  • webchick committed 30c2afe on 8.x
    Issue #697760 by sun: Replace getInfo() in tests with native phpDoc +...
tim.plunkett’s picture

FileSize
18.04 KB

Do we have a follow-up for all of the duplicate groups now?

alexpott’s picture

hass’s picture

Status: Fixed » Closed (fixed)

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

penyaskito’s picture

+++ b/core/modules/simpletest/simpletest.module
@@ -129,40 +130,34 @@ function _simpletest_format_summary_line($summary) {
-  $phpunit_tests = isset($test_list['UnitTest']) ? $test_list['UnitTest'] : array();
-  if ($phpunit_tests) {
+  if (!empty($test_list['phpunit'])) {
     $phpunit_results = simpletest_run_phpunit_tests($test_id, $phpunit_tests);

This broke running phpunit tests from the UI, as phpunit_tests is never initialized. Filled #2318257: PHPUnit cannot be run from the simpletest UI.

YesCT’s picture

the doc page for tests (simpletests) really needs to be updated to reflect the change this issue did.

https://www.drupal.org/node/325974