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
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | 3360167-3.patch | 2.27 KB | longwave |
| #2 | 3360167-2.patch | 2.27 KB | longwave |
Comments
Comment #2
longwaveTwo groups of changes here:
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.
Comment #3
longwaveIn fact let's use a better check for zlib.
Comment #4
mondrakeSkip 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.
Comment #5
longwaveThe "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.
Comment #6
longwaveNote 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.
Comment #7
longwaveTBH 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...
Comment #8
mondrakeYeah, I assume we could debate forever...
Looks good to me.
Comment #10
catchCommitted d68a28b and pushed to 11.x. Thanks!