Contrib PSR-4 PHPUnit tests are not picked up by PIFR

FWIW, PSR-4 PHPUnit tests can be uniquely identified, because they are contained in a /tests/src/ directory.

It would make more sense to me to add that as an explicit condition...

That said, that entire code in PIFR predates the introduction of PHPUnit tests. — I'm surprised that PIFR runs any PHPUnit tests at all. That's pure coincidence.

IMO, we should stop trying to manually discover tests of a particular module in PIFR. Instead, this logic MUST be delivered by run-tests.sh itself. Testbots SHOULD just pass --module page_manager (or alternatively, a file path to an extension).

#1683884: Update, simplify, and speedup run-tests.sh for PSR-0, and allow to run all tests of a module did introduce some basic code for doing that already, but to my knowledge, it doesn't work correctly yet + PIFR doesn't use it yet.

The new test discovery code from #697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit) will certainly help with that, too.

Ultimately, PIFR shouldn't try to assume anything about the tests to run. Core should control the test suite.

Glad to hear that it still works (- even though that code is not actively used nor tested in any way ;))

However, after learning about the filtering options supported by PHPUnit, I'm no longer sure whether adding the --module option in its current form was "right."

1. --module obviously means it only works for modules.
2. PHPUnit uses a more sane approach of filtering/limiting by a directory path.
3. We could/should translate that into run-tests.sh --extension <path>

That is, because run-tests.sh has to perform an ExtensionDiscovery either way, in order to register all available namespaces (because extensions are not necessarily installed/enabled).

That discovery process merges the results of all extension types into a single list of available extensions either way already. (hard-coding the assumption that all extension names must be unique, regardless of type)

Therefore, a single --extension parameter could either point to an extension name or to its pathname. The former would be equal to the current --module behavior, the latter would be more in line with PHPUnit.

How to move forward here:

1. I think we should rename --module to --extension in Drupal core first.
2. Decide whether that parameter takes an extension name or pathname (or make it smart enough to understand both?)
3. Make PIFR use that parameter for testing D8+ contrib projects.

@sun (#3)

IMO, we should stop trying to manually discover tests of a particular module in PIFR. Instead, this logic MUST be delivered by run-tests.sh itself.

I agree with this approach, but don't know how much work this is going to be.

So I want to comment on Tim's patch in #2, which seems more promising for a quick solution. This could be temporary, but it would unblock #2247287: Drop automatic PSR-0 support for modules.

FWIW, PSR-4 PHPUnit tests can be uniquely identified, because they are contained in a /tests/src/ directory.

The correct discovery procedure for D8 would be to first look for modules (identified by modulename.info.yml), and then look for tests/src/ folders within these modules. Very similar to what the switch-psr4.sh script does. And probably any other discovery code we have in D8.

Important thing: We still need to support tests/Drupal/.. with PSR-0 for Drupal 7!
Maybe we could have dedicated methods for different Drupal versions, based on the *.info or *.info.yml files being discovered.

Not sure if this is really the correct approach, so I am posting some pseudocode before a patch.

foreach (file_scan_directory($dir, '\.info$') as $info_file) {
  $module_dir = dirname($info_file);
  $module_name = basename($info_file);
  // Find D6 / D7 tests within this module.
  $this->findTestsInModuleD6D7($module_name, $module_dir);
}

foreach (file_scan_directory($dir, '\.info\.yml$') as $info_yml_file) {
  $module_dir = dirname($info_file);
  $module_name = basename($info_file);
  // Find D8 tests within this module.
  $this->findTestsInModuleD8($module_name, $module_dir);
}

Larger changes (including the plan to replace this manual scan with native run-tests.sh functionality) should be handled separately.

Attached patch is same as @tim.plunkett's — just using stripos() + adjusting docs accordingly.

Would be good to get this committed and deployed before DrupalCon.

This should be live on all testbots within 30 minutes.

This looks like it could lead to false positives.
It will assume that e.g. tests/modules/foo/src/Foo.php is seen as a test class, when in fact it is just a regular class within a test module.

What about this instead:

if (FALSE !== strpos($file->filename, '/tests/src/') || FALSE !== strpos($file->filename, '/tests/Drupal/') {

Looks like puppet wasn't pulling changes as intended ... should be working now.

Thanks for the report!

Needs review for followup in #14.

adding "infrastructure blocker for Drupal 8.0.0" tag, since this blocks the drupal 8 core issue: #2267715: [meta] Drupal.org (websites/infra) blockers to a Drupal 8 RC1, so that the infrastructure blockers to d8 release (in issue queues outside of the core queue) is accurate. Remove the blocker tag when this issue is fixed, and update 2267715.

I can give some definite examples of false positives. There have been about 49 tests that just bail out for the d7-3.x views module since this was implemented.

The issue that Im seeing is that there is a file under the views tests directory - /tests/templates/views-view--frontpage.tpl.php, which I believe is confusing the test runner because a. It is not a test. an b. run-tests.sh is looking for two dashes in a row to indicate an argument name, and doesnt know what to do with frontpage.tpl.php as an argument.

https://www.drupal.org/node/2456043 was the issue that brought a user to #drupal-infrastructure
https://qa.drupal.org/pifr/test/1005698 are the test results
https://www.drupal.org/node/2458143 issue filed about this bug.

https://www.drupal.org/node/2159347#comment-9486479 is another example where a d8 backport to d7 cant really be tested.

I think one of two things need to happen here. In the short term, can anybody who is savvy with views testing explain the /tests/templates/views-view--frontpage.tpl.php and if it needs to be there? I tried removing it and submitting a patch, and the testbots picked it up and ran it fine with five failures (https://qa.drupal.org/pifr/test/1005713)

Do we need to patch the regex in run-tests.sh?

So, according the comments, of findTestFiles in pifr_simpletest.client.inc there is no distintion for D7 and D8, so currently for D7 wach file with .php extension will be considered a test file.

I think one of two things need to happen here. In the short term, can anybody who is savvy with views testing explain the /tests/templates/views-view--frontpage.tpl.php and if it needs to be there? I tried removing it and submitting a patch, and the testbots picked it up and ran it fine with five failures (https://qa.drupal.org/pifr/test/1005713)

This file is there because views needs to test the template system inclussion. We could fix the inclusing by moving the template inside a vendor directory if this patch is not accepted.

I'm including the git diff -w ouput on the 2273481-no-spaces.do-not-test.patch file to make easy to understand the code provided here.

+      // Since we do not want to hard-code too many structural file/directory
+      // assumptions about PSR-0/4 files and directories, we check for the minimal
+      // conditions only; i.e., a '*.php' file that has '/Tests/' in its path.

Imo it should use the same rigid logic as core test discovery. Why should we be overly permissive here?

@donquixote That code you mention came from #1674290: PSR-0 tests in contrib modules are not found

@sun explained his reasons here: https://www.drupal.org/node/1674290#comment-6207792

Seems like a simple solution to fix a big problem.

RTBC +1

dagmar++

So good to see you fixing actual problems out there!

Re #21 -

The code was expanded to support psr0/4 testing namespaces for d8 (https://www.drupal.org/node/1543796), but it also unintentionally added that support to d7. Im concerned that there may be examples of d7 modules that are using psr0/4 namespacing for their tests, and if we apply this patch it will disable testing for those modules.

Can we be sure there are no examples in contrib where somebody has added psr0/4 tests to D7 modules? I could probably create a working checkout of everything in the git repos to do a search, but that would take a fair amount of time for me to get to.

An alternate workaround to get this working in views, specifically, is to disable that one test that relies on that template file, or, alternately, put that template file in a 'test-assets' directory or something that isn't subordinate to a /Tests/ directory. (http://cgit.drupalcode.org/project_issue_file_review/tree/review/simplet...)

FYI in #2450447: Drupal QA is broken for Views 7.x-3.x dagmar posted a patch to move the test template file into a different subdirectory, I guess some guidance is needed to pick which direction should be taken.

Also, I asked on twitter about D7 modules using PSR0/4 tests and was told that OpenLayers, VarDumper and Service Container do.

One more: Commerce Discount: #2481201: Update and fix the tests!

Another: Commerce Reports (per Matt Glaman)

PIFT/PIFR no longer need to fix this as it is now fixed in drupalci.

Drupalci will both find simpletest tests with the extension of .test, as well as any test files that end in .php that are found by the simpletest module itself.

When doing discovery of tests in drupalci, it explicitly excludes .tpl.php and .api.php files from files it thinks may be tests.

Additionally, the test discovery process for drupalci no longer uses the --file argument, so the -- in the filenames is no longer a problem.

yay.