Make run-tests.php compatible with php 7.0-7.2 by TysonAndre · Pull Request #3981 · php/php-src

TysonAndre · 2019-03-23T21:29:31Z

Motivation:
As an extension author, I want to speed up running tests in php <=7.3,
both locally and in CI (e.g. when running with valgrind).
This can be done by manually copying php 7.4's run-tests.php script
to replace the one generated by phpize

list() doesn't work in php 7.0
negative string offset doesn't work in php 7.2
This fix has been tested with php 7.1 and 7.0 on linux

If run-tests.php can be copied from php-src without any manual patches,
that would be the easiest.

Related to #2822 and #3838 - I didn't see any discussion for/against compatibility
with older php versions in that PR.
except for https://github.com/php/php-src/pull/3838/files#r257469673

Both of the changes requiring syntax newer than 7.0 were introduced in that PR.

Motivation: As an extension author, I want to speed up running tests in php <=7.3, both locally and in CI (e.g. with valgrind). This can be done by manually copying php 7.4's run-tests.php script to replace the one generated by `phpize` - list() doesn't work in php 7.0 - negative string offset doesn't work in php 7.2 If run-tests.php can be copied from php-src without any manual patches, that would be the easiest. Related to php#2822 - I didn't see any discussion for/against compatibility with older php versions

KalleZ · 2019-03-23T21:31:04Z

I think the original idea was to keep run-tests.php in sync to as big an extend as possible, so I think it makes good sense to do this

petk · 2019-03-23T21:40:49Z

Friendly reminder: PHP 7.0 is dead...

TysonAndre · 2019-03-23T21:44:22Z

Friendly reminder: PHP 7.0 is dead...

I'm aware of that, but the work needed to fix this syntax error is negligible and some extension authors haven't dropped php 7.0 from their major version releases (e.g. for CI). If that position is taken by a majority of reviewers, I'm fine with taking that back out.

Off-topic: I noticed that -j1 will start a single worker process (i.e. call run_all_tests_parallel) instead of the default (running tests from the main process). I can't think of a realistic use case for wanting that (e.g. would happen if nproc returned 1) and didn't see anything earlier

$workers !== null could be $workers >= 2 instead

	if ($workers !== null && !$workerID) {
		run_all_tests_parallel($test_files, $env, $redir_tested);
		return;
	}

KalleZ · 2019-03-23T21:46:06Z

@petk its a small sacrifice for cross compatibility, it is even better if it helps extension authors. As you can see, the patch is relatively simple and fair enough. Things like these are usually done on a case by case basis and the win for people doing cross versions, despite we do not actively support a certain branch.

I therefore think it is a fair compromise to do this

KalleZ · 2019-03-23T21:47:46Z

Tagging @hikari-no-yume for the parallel thing you mentioned @TysonAndre

TysonAndre · 2019-03-23T22:52:43Z

This approach works in Appveyor/Travis. The speedup for Travis with valgrind is noticeable but not as much as I'd expect, but I assume that's just because the VM in AWS is being throttled (virtual CPUs, not real CPUs). Appveyor takes 3 seconds with/without it, but works.

https://ci.appveyor.com/project/TysonAndre/igbinary-bemsx/build/job/ha3kop5huw9agtf7

More off-topic discussion:

#2822 (comment) " I suggest you add something like --no-ansi, and turn it on on windows."

It seems like Appveyor's unicode support is lacking ; that's one argument in support of a --no-ansi equivalent. It literally looks like the below text in my browser (Chrome/Firefox). Maybe I misconfigured something when running the batch script generated in .appveyor.yml, though

=====================================================================
====ΓÜí∩╕Å===========================================================ΓÜí∩╕Å====
====ΓÜí∩╕Å==== WELCOME TO THE FUTURE: run-tests PARALLEL EDITION ====ΓÜí∩╕Å====
====ΓÜí∩╕Å===========================================================ΓÜí∩╕Å====
Spawning workersΓÇª 1 2 ΓÇª done!
====ΓÜí∩╕Å===========================================================ΓÜí∩╕Å====
PASS Check for igbinary presence [C:\projects\igbinary\tests\igbinary_001.phpt] 
TEST 1/78 [ΓÜí∩╕Å[2/2 concurrent test workers running]ΓÜí∩╕Å]
                                                              
PASS Unserialize invalid data [C:\projects\igbinary\tests\igbinary_030_php7.phpt] 
TEST 2/78 [ΓÜí∩╕Å[2/2 concurrent test workers running]ΓÜí∩╕Å]
                                                              
SKIP Unserialize invalid data (php 7.2+) [C:\projects\igbinary\tests\igbinary_030_php72.phpt] reason: php 7.2+ required
TEST 3/78 [ΓÜí∩╕Å[2/2 concurrent test workers running]ΓÜí∩╕Å]
                                                              
PASS Check for null serialisation [C:\projects\igbinary\tests\igbinary_002.phpt]

(and it may be nice to have a minimal version without the concurrent test workers running count, e.g. when NO_INTERACTION=1 is set or via some new config/env flag)

TysonAndre · 2019-03-24T00:17:42Z

Other notes on run-tests.php in php 7.4-dev:

https://git.php.net/?p=php-src.git;a=commitdiff;h=03f15f705ad30ce2dde7f7b2ab087a132b859ba0 made memory leaks start failing tests at the same time --show-mem was implemented. Some libraries might have false positive memory leaks (or real memory leaks that are non-trivial to resolve)

This seems reasonable for a default for the majority of projects
I couldn't find discussion of this so I'm mentioning it here.

Would it be reasonable to add IGNORE_MEMORY_LEAKS=1 as an environment variable option to make test suites pass if they leaked memory but otherwise passed. (or a similar CLI option) (in two places, both for parallel and regular builds)

The patch would be runkit7/runkit7@d2981e8 or something along those lines
I noticed this change to run-tests.php exit codes when setting this up for Run tests in parallel in Travis CI runkit7/runkit7#178 (a fork supporting php7 - "The runkit extension provides means to modify constants, user-defined functions, and user-defined classes.". The memory leaks are non-trivial to resolve)

hikari-no-yume · 2019-03-24T01:06:19Z

Regarding -j1, I use it to debug the parallel testing system to see if a test failure is because of parallelism or because of the different execution mode.

krakjoe · 2019-03-24T08:04:37Z

Would it be reasonable to add IGNORE_MEMORY_LEAKS=1 as an environment variable option to make test suites pass if they leaked memory but otherwise passed. (or a similar CLI option) (in two places, both for parallel and regular builds)

No, I don't think that's reasonable: If you are running tests with valgrind you are checking for memory errors and that includes leaks. If you want to consider memory errors (including leaks) as unimportant for a particular project, then you should allow_failure in your travis configuration, rather than adding another env var to run-tests.

Maybe a bit of rationale to explain my position: False positives can be suppressed by configuration of vg, so false positives are a non-issue. The issue you have is that your project leaks and or has memory errors that you want to ignore, but beyond that, you want other people relying on your project to ignore them - that's my issue. Continuous integration, and testing, are a way to determine the suitability of a project for use, and if your project has memory errors and leaks then anyone intending to use your project should be aware of that, and you should be trying to fix those errors, not finding ways to suppress them, and give others the false impression that it's in a better state than it actually is. Setting allowed failures still allows the build to pass, but for anyone looking, they can see instantly that there are issues in some builds, without digging through env vars and or run-tests to determine the actual state of the project.

petk · 2019-03-24T11:28:50Z

How about PHP 5.6 support? There are a lot of 5.6 out there also...

KalleZ · 2019-03-25T12:51:45Z

@petk Let's be reasonable here really, we might as well support these as they are in the same major version, just because X is available does not mean we need Y because its also used etc. If you have ever written an extension and maintained it over a cross set of versions of PHP, you will know how much something so simple eases it.

nikic · 2019-03-25T13:00:16Z

Merged as 936356b, thanks.

nikic · 2019-03-25T13:24:56Z

Also 3c61972 and 071ffa2.

petk · 2019-03-25T18:29:03Z

@petk Let's be reasonable here really...

Noted... Added run-tests.php to the list of "php": ">=7.0" requirements then. Maybe for PHP-8.0 (but this is also a bit soon), maybe 8.1 branch in 2021 might be using syntax from 7.1 (released in dec 2016) already... Otherwise this is a genuine concern here. In PHP-7.4 branch I would expect a tool to support syntax that is of the same branch. For Git merging issues I might even understand. For running this on outdated EOL installations, not so much.

Anyway this entire run-tests.php script is in need of a complete rewrite to be more proper anyway. So all is good :)

TysonAndre changed the base branch from master to PHP-7.4 March 23, 2019 21:29

KalleZ approved these changes Mar 23, 2019

View reviewed changes

nikic closed this Mar 25, 2019

petk mentioned this pull request Mar 25, 2019

Add minimum required PHP version to run-tests.php #3986

Closed

TysonAndre mentioned this pull request Jul 14, 2019

Check for Windows utf-8 support in run-tests.php #4404

Closed

TysonAndre deleted the run-tests-php-7.0-compat branch October 7, 2019 13:33

nikic mentioned this pull request Oct 28, 2019

Define IS_WINDOWS in the test runner #4866

Closed

Uh oh!

Conversation

TysonAndre commented Mar 23, 2019

Uh oh!

KalleZ commented Mar 23, 2019

Uh oh!

petk commented Mar 23, 2019

Uh oh!

TysonAndre commented Mar 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KalleZ commented Mar 23, 2019

Uh oh!

KalleZ commented Mar 23, 2019

Uh oh!

TysonAndre commented Mar 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TysonAndre commented Mar 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hikari-no-yume commented Mar 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krakjoe commented Mar 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petk commented Mar 24, 2019

Uh oh!

KalleZ commented Mar 25, 2019

Uh oh!

nikic commented Mar 25, 2019

Uh oh!

nikic commented Mar 25, 2019

Uh oh!

petk commented Mar 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

TysonAndre commented Mar 23, 2019 •

edited

Loading

TysonAndre commented Mar 23, 2019 •

edited

Loading

TysonAndre commented Mar 24, 2019 •

edited

Loading

hikari-no-yume commented Mar 24, 2019 •

edited

Loading

krakjoe commented Mar 24, 2019 •

edited

Loading