Problem/Motivation

I think there is further simplification that can be done in UpdatePathTestBase (zlib check, update URL generator, etc) but that can be deferred to a followup.

(@longwave in https://www.drupal.org/project/drupal/issues/3354063#comment-15013265)

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#3 3360167-3.patch2.27 KBlongwave
#2 3360167-2.patch2.27 KBlongwave

Comments

Spokje created an issue. See original summary.

longwave’s picture

Status: Active » Needs review
StatusFileSize
new2.27 KB

Two groups of changes here:

  1. The zlib check dates back to Simpletest, and previously ran half the update tests then failed if zlib was not available. If zlib is not available we can't import the compressed database dumps, so we might as well just skip the test early.
  2. There are four other properties in UpdatePathTestBase that have been refactored away in the move from Simpletest to PHPUnit, so let's remove their declarations.

I previously thought we should refactor away the updateUrl property, but this pattern is used elsewhere in update tests, so I think it can be left alone here.

longwave’s picture

StatusFileSize
new2.27 KB

In fact let's use a better check for zlib.

mondrake’s picture

Skip or fail? IMHO skipping make sense when you have 'alternatives'. For instance, you skip the SQLite-specific tests when you're running a different database. Or you skip testing an image format if you do not have the extension able to manage it. But here, if no zlib installed, a significant portion of the tests will simply be ignored - and who knows what a bot has really installed.

longwave’s picture

The "official" way of doing this is with the "@requires extension zlib" annotation, which also skips, but I don't think that works in base classes.

Overall I still think "skip" is appropriate. If you're running the whole test suite in a specific environment, then you expect tests to pass against what is available in that environment. If you're running individual tests or a small group, the report will tell you that the tests were skipped, and you can investigate to find out why.

In a similar way WebDriverTestBase skips instead of fails if it can't start Mink or connect to webdriver, which in some cases means that none of the functional JavaScript tests are actually run.

longwave’s picture

Note that I previously argued the other way for WebDriverTestBase but was convinced by @alexpott and the PHPUnit docs in #3187577: FunctionalJavascript tests should fail when ChromeDriver is not running that skip is correct.

longwave’s picture

TBH after reading https://phpunit.de/manual/6.5/en/incomplete-and-skipped-tests.html again, maybe we should use "incomplete" to identify random fails and other non-working tests, and "skip" to identify environment specific issues...

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Yeah, I assume we could debate forever...

Looks good to me.

  • catch committed d68a28b4 on 11.x
    Issue #3360167 by longwave, mondrake, Spokje: Simplification in...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed d68a28b and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

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