Implement @requires and @dependencies within TestBase, mark tests as skipped

Hi there ... Thanks for the patch, I rather like this feature. Here's what I've tested so far:

• Tested on D8 environment pre-patch testing book tests
• Apply patch and rerun book tests: all OK
• Test with a basic checkRequirements() implementation on book tests that returns FALSE. As expected - tests are allowed to proceed, as the return value is not an array with "errors" set
• Tested book tests with a checkRequirements implementation that returns array('errors' => 'test message')

On the whole, it's looking good, but I'm not hugely keen on this line:

if ($access !== TRUE && isset($access['errors']) && count($access['errors']) > 0) {

As what it's checking ultimately is just the same as:

if (!empty($access['errors'])) {

Why not just go with the shorter version? (It won't emit notices)

Also this line:

        'function' => 'n/a',

Is actually covered by the default in DrupalTestCase::insertAssert (default is 'Unknown', likewise default line number is 0, so we shouldn't need to set either of these - I'd recommend removing both the function and line number here. I've just tried doing that, and the result is the same, except that the assert shows the default function name of "Unknown" instead of "n/a")

• Checked arguments of insertAssert() - all sane
• Checked diff of contents of the else {} block, from "foreach ($class_methods" - all the same as the original

Hey @btmash, just reviewed your patch, my only suggestion would be simplify the return value of checkRequirements - perhaps have it return an array only, an empty array by default indicating no problems and alternatively an array of errors if any requirements are not met. It'll make your logic on line 47 of the patch simpler and reduce a possible element of confusion. Perhaps needs a little more on the usage in method comment block too.

Ah, missed your post @Alan Evans, setting to needs work.

Yep, I'd tend to agree - making that return value always an array would be a good way to simplify things

Thanks for the update! Checks done:

• Re-read the patch - looks good
• Run book tests with no changes - OK
• Run book tests with checkRequirements() implementation returning empty array - OK
• Run book tests with checkRequirements() returning test message - OK
• Run multiple tests including book tests with checkRequirements() returning test message - OK (fails correctly)

Was going to do a full test run with and without checkRequirements, but I realise this is going to take hours on my laptop, so I think we'll just trust the bot.

One slight annoyance is that the batch test run reports fails 0, passes 0, exceptions 0 when failing on requirements, but the end test result report is correct. I don't think that's anything to do with your patch though - the other DrupalTestCase::insertAssert in prior code would do exactly the same thing (the one that checks for completion of individual tests). It looks to me like that's just how insertAssert is implemented that means that the batch runner doesn't know about the fail until the end.

Going to mark this RTBC for now, can always go back if there are objections.

Sorry, I need 1) a better issue summary here, and 2) a better explanation of what is being changed and why (which circles back into 1)). Right now, I can't make any sense of this patch.

Thanks for the improved summary.

Looks like this patch

1) needs more in-depth reviews from simpletest/testing-framework maintainers but also OO-masters, and

2) could use an example implementation (I'm quite sure we already have tests in core that contain an early-return based on an environment condition)

Wrong post.

Yes, the test should fail if GD is not installed rather than skip especially since core only ships with gd support.

I like this patch and thinks it makes great sense. However, I am neither a simpletest/testing-framework maintainer nor a OO-master.

The only issue I noticed was an incorrect indenting of the word Array below @return line.

While awaiting review from others, it make sense to identify the other cases in existing tests where this new function could be used. I would not code them up yet in the patch, but the information will be helpful for when the final patch is rolled and hopefully applied.

Looks like the summary's been added; untagging.

I've taken another look over the latest patch and here are the results:

• Still no actual practical difference in the main foreach loop which runs the tests (diffed the loops to check), same as previous patch. Good.
• There is nothing hugely complicated from an OO perspective, so I disagree that it needs more than a glance from the OO gods. That said, I wonder whether checkRequirements really needs to be protected rather than public. I never see this called from outside the class or subclasses, so I don't see a need for public. General rule of thumb is allow the lowest possible level of access - private if possible, then see if you can use protected, public functions should be part of the external-facing interface, which this doesn't really qualify for
• I've retested the patch using protected - all runs ok
• The patch currently only applies with minimum of -F3 ... needs rerolling
• Ran patched image manipulation and upgrade tests to verify: all passed
• Re-ran both with a faked fail-requirements case: all failed nicely
• Your removal of similar requirements-checking code from within the tests themselves looks good
• Checked example GD checkRequirements implementation - looks good
• Checked if upgrade requirements are reasonable: in general this looks good, but extension_loaded('zlib') is the more direct way of checking this. I like the use of image_gd_check_settings in requirements, as it uses a function of the toolkit itself within the requirements check, but this is not really the case with function_exists('gzopen'): it checks for the existence of a function that is not used within the test, the test itself depends on another feature of zlib (the stream wrapper).
• Ran another random unaffected test to check it doesn't affect other tests: OK

Remaining TODO:

• consider using extension_loaded() for the zlib check, or argue the case for function_exists('gzopen')
• consider changing checkRequirements method access to protected, or argue the case for public
• Get it looked at by boombatower or chx
• Reroll patch for latest d8 (problem I think was 1 hunk in the upgrade test - just needs a little extra fuzz to get the patch applied)
• Let d.o do a full test run to validate the final patch

Turns out the patching issue was a spelling correction in a comment in upstream which happened to coincide with one of the context lines of the patch.

Here's a rerolled version of the patch. The only changes are:

• Applies to latest D8
• Uses protected instead of public access for checkRequirements(), but I'm happy to revert to public if there's a case for it.

Thanks for working on this patch. This will need to be rerolled on account of #22336: Move all core Drupal files under a /core folder to improve usability and upgrades. Also, here are a few minor points about the patch's code style.

+++ b/modules/simpletest/drupal_web_test_case.phpundefined
@@ -84,6 +84,16 @@ abstract class DrupalTestCase {
   /**
+   * Check for matching requirements for DrupalTestCase.
+   *
+   * @return
+   * Array of errors containing a list of unmet requirements.
+   */

+++ b/modules/simpletest/tests/image.testundefined
@@ -217,6 +217,19 @@ class ImageToolkitGdTestCase extends DrupalWebTestCase {
   /**
+   * Check that the GD toolkit is enabled in order for the tests to run.
+   */

+++ b/modules/simpletest/tests/upgrade/upgrade.testundefined
@@ -28,19 +28,23 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
   /**
+   * Check that zlib is enabled in order to run the upgrade tests.
+   */

The docblock summaries should use a third-person present indicative verb (in this case, "Checks" instead of "Check.) We're currently cleaning this up as a part of #1310084: [meta] API documentation cleanup sprint, so we should make sure new dockblocks agree with the standard as well.

Also, there should be two spaces of indentation after @return, like so:

@return
  Array of stuff...

Tagging as novice for the task of rerolling the Drupal 8.x patch and making the code style corrections above.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

Applied d8 /core patch change. In some of the files, D8 path updates in the code affected patch apply. Modified patch to include those references to /core/includes/update.inc' and /core/update.php'
Added updates per #24

Patch looks good to me. Do we also need to add a test to simpletest's self-tests that tests whether missing dependencies properly cause failed tests? :)

Thanks @BTMash. Can you also upload the test separately to expose its failure? (If you put the test-only patch first and the combined patch second in the same comment, testbot won't set it to NW for the intended failure.)

Right, so this is new functionality and therefore exposing test failure with a separate patch doesn't really make sense. Sorry. :)

Seems like a solid plan. We had a brief conversation in IRC and all seems well.

#29: 1273478_29.patch queued for re-testing.

I'm betting it needs to be rerolled for #1212992: Prevent tests from deleting main installation's tables when parent::setUp() is not called.

Please change $access to $missing_requirements and the $error accordingly to $missing_requirement and then feel free to RTBC, it's just a variable rename, I am fine (and so was Jimmy above) with the patch.

Thanks; this variable naming makes the code clearer and more self-documenting.

Committed/pushed to 8x, thanks!

Will need a re-roll for 7.x (but might want to wait for table deletion issue first to save double re-rolling)_.

Here's a D7 backport. Not a clean reroll so NR. (It does at least have the same number of fingers and toes.)

Looks good to me.

Hm. I really wonder why we didn't make checkRequirements() a static method.

Right now it's not, which means you need to instantiate the test first, in order to figure out whether it can be run in the first place. Whereas we can discover tests and get their info without instantiating. That slightly contradicts the meaning of "requirements."

I'd actually prefer to bump this back to D8 and make that method static. Potentially also consider to auto-invoke it directly from __construct() and throw an exception, so code that doesn't check the requirements correctly blows up.

Looked at this again, and it seems like the intention is to make tests with unmet requirements explicitly fail.

This still means that such test classes are (successfully) instantiated and attempted to run. Whereas I'm pretty sure that we're able to check and return requirements either even before instantiating, or right within the class instantiation (i.e., __construct()). Architecturally, that would make more sense to me.

Aside from that, I don't really get why we're re-inventing Exceptions with a custom array of error strings, which are manually inserted as errors into the test log afterwards?

However, I don't want to hold off progress for Testing system improvements... the only reason for raising the concerns here and not in a new issue is that we're about to backport this API addition to D7, and so, while we can possibly adjust D8 afterwards, that's not going to be possible for D7 anymore.

---

To merely outline the direction I mean, see attached patch. It's just a start to show the approach using exceptions, and doesn't involve __construct(); it's not a final patch in any way. Left indentation and whatnot alone, so it's easier to see the functional changes.

Recategorizing.

The current implementation throws a test failure when requirements are not met, but that is bogus and also insufficient. A test that lacks requirements neither means success nor a failure. #843114: DatabaseConnection::__construct() and DatabaseConnection_mysql::__construct() leaks $this (Too many connections) requires a way to skip a test when not running on MySQL.

Took me some time to find the docs for the corresponding functionality in PHPUnit:
http://www.phpunit.de/manual/current/en/incomplete-and-skipped-tests.html

So scratch the exceptions approach I mentioned earlier; what we need is:

Table 9.2. API for Skipping Tests

updating summary.

Minimally revising this issue to account for the latest and greatest efforts.

One of my topmost/primary concerns is that the current test runner code simply skips classes with unmet dependencies during the test discovery already, so you do not even see that additional tests may exist but might have been skipped; i.e., they're not even listed as available tests - the test runner simply presumes they wouldn't exist at all.

My plan is to introduce full parity for Simpletest tests with PHPUnit tests in D8. That not only means that you'll be able to specify "@requires PHP 5.5" or "@requires extension curl", it also means that Drupal module dependencies will be handled in the same way; i.e., "@requires module libraries".

If an environment requirement is not met when a test is attempted to be run, the test will be marked as "skipped".

Next to custom Drupal extension type requirements, the goal is to fully adopt/resemble PHPUnit's @requires annotation for skipping tests.

All of that happens at test execution/runtime, so it doesn't affect PIFR - aside from new assertion result categories. I briefly talked to @jthorson about necessary changes on that front already.

In concrete terms, this means that the list of assertion categories in TestBase::$results will be extended by "skipped", "incomplete", and "risky" — fully adopting all of PHPUnit's possible assertion result categories.

The meta data will be delivered by TestDiscovery::getTestInfo(), by courtesy of #697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit)

We may also consider to add explicit corresponding methods to TestBase (cf. #53), but that may also happen later and is not a priority for now.

Postponing on the parent issue.

Just unassigning issues that haven't been developed for a bit in the D8 queue.

Related: #2728579: Explicitly skip @requires module in PHPUnit Kernel and Browser tests which checks for @require module on phpunit-based tests.

Rescoping a bit because the IS was way behind the times. Moving to 8.4.x because, even though it's a testing improvement, it's disruptive.

This patch does not mark tests as skipped, because Simpletest doesn't know what 'skipped' means (yet). Scope for adding a skipped status to Simpletest can be figured out here. Should it be in the patch? Should it be another issue that blocks this one?

This patch is a WIP. Please comment. :-)

Duplicate: #2294379: Tests with dependencies do not show up if the dependencies are present

#2905007: Allow run-tests.sh to report skipped/risky/incomplete PHPUnit-based tests adds some infrastructure to run-tests.sh to support skipped tests. That will make it easier to support them here eventually.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

With PHPUnit 10 getting extremely strict about the use of annotations to only those they support, this is pretty much outdated now.