Fix #79639 - Preserving the html report options in parallel worker mode #5632

dragoonis · 2020-05-27T04:05:25Z

Consider this the first of many fixes/improvements I'm looking to make on the testing side, with a particular focus on PHP8.

DX (Developer Experience) is going to be centric to why I do things, and not just modernizing code for the sake of it.

I've initially focused on the HTML report, because it was broken in worker mode, but I'll be reviewing other areas in worker mode too, and applying updates respectively.

Once this is finally merged, after a sanity check, I'll be coming back for more, for example our junit and stdout output methods.

run-tests.php

nikic · 2020-05-27T07:35:40Z

As a higher level question, I wonder if we could just drop the --html mode? This is the first time I even heard about it, and it's not clear to me why anyone would want to have that.

dragoonis · 2020-05-27T20:48:07Z

@nikic I respect your point of view. I'd prefer that we took the approach of not removing this functionality as it's currently in place for some time now, and working just fine in non-parallel mode. Removing would be a regression in run-tests.php. Neither of us know who is and isn't relying on this feature so it's safer to not rip it out.

Going forward, I intend to have wider discussion around a richer developer experience by way of a web-based report, and I'd much prefer to keep these calls in place so I have the opportunity to make that happen. If we rip these out now then I'll have to add them all back in again later and that's not a good use of my time, which can be put into other areas of the project.

In conclusion, given we're not using --html on our CI then it's not harming us right now, and there's bigger problems in this script than an optional --html flag, so with that in mind I hope you'll be happy to retain this feature.

Thanks.

Girgias

I don't know much how the test runner works, but it looks reasonable other than the fact that I would prefer the usage of type declarations.

Girgias · 2020-05-27T20:44:10Z

run-tests.php

+    /**
+     * @param string $outputFilename
+     */
+    public function setHtmlOutputFilename($outputFilename)


Why is this not using a type declaration?

Suggested change

public function setHtmlOutputFilename($outputFilename)

public function setHtmlOutputFilename(string $outputFilename)

See the conversation below. Making this incompatible with php 5.x isn't something we've discussed. Once we've had that dsicussion then I'll update this file, respectively.

Girgias · 2020-05-27T20:44:34Z

run-tests.php

+    /**
+     * @return bool
+     */
+    public function isHtmlEnabled()


Suggested change

public function isHtmlEnabled()

public function isHtmlEnabled(): bool

Girgias · 2020-05-27T20:46:05Z

run-tests.php

+    /**
+     * @var null|string
+     */
+    private $htmlOutputFilename = null;


Suggested change

private $htmlOutputFilename = null;

private ?string $htmlOutputFilename = null;

This one I'm less sure as I don't know how far back in compatibility we need to go, but if we are only targeting master this would make sense IMHO.

Per the comment at the top of the file, this script is currently compatible with PHP 7.0. We could discuss raising the constraint, but unlikely to go to 7.4. (The test runner can be invoked with a different PHP version than it is testing. I believe @TysonAndre also expressed a desire to use newer test runners on PECL extensions that support old PHP versions, so it's possible to use new features.)

I didn't add any types because I wanted to make it PHP 5.x compatible.

So what I'd like clarification on is: should I be adding any 7.0 features to this script? If so I'll go ahead and add types. Please clarify. Thanks

Per the comment at the top of the file, this script is currently compatible with PHP 7.0. We could discuss raising the constraint, but unlikely to go to 7.4. (The test runner can be invoked with a different PHP version than it is testing. I believe @TysonAndre also expressed a desire to use newer test runners on PECL extensions that support old PHP versions, so it's possible to use new features.)

The most significant new feature to me was the parallelism to take advantage of all cores on a test runner -j N.

I'm fine with raising the constraints - PECL projects which did need a run-tests.php that worked with 7.0-7.4 (and probably 8.x) could just copy the run-tests.php from the PHP-7.4 branch.

This file is already incompatible with php 5.6 - I don't think anyone is planning to support php 5.6 in the latest versions of run-tests.php.

It aimed for 7.0+ in one release, but that's already available, and can get raised.

I don't have a personal need for newer features right now, but I could imagine others wanting it

$environment = $_ENV ?? array(); // line 190 uses php 7's null coalescing

In that case i will update this code to give us the type safety advantages
of php 7. Thanks

Girgias · 2020-05-27T20:47:08Z

run-tests.php

@@ -1412,7 +1414,7 @@ function run_all_tests_parallel($test_files, $env, $redir_tested)
    // Don't start more workers than test files.
    $workers = max(1, min($workers, count($test_files)));

-    echo "Spawning workers… ";
+    echo "Spawning workers ... ";


This change is kinda unrelated and I'm not sure if it makes sense to change

nikic · 2020-05-27T21:20:28Z

@nikic I respect your point of view. I'd prefer that we took the approach of not removing this functionality as it's currently in place for some time now, and working just fine in non-parallel mode.

The argument here goes both ways: It is telling that we've been making heavy use of parallel run-tests for more than a year, and this is the first time anyone noticed that it's incompatible with --html.

I don't see much value in the HTML report functionality, and I do think we should give some consideration to this now, especially if you were planning further work in this area. I would not want that work to go waste due to a target audience mismatch.

There are currently three commonly used output formats:

By default, and what is used in day-to-day development, are the .diff etc files produced for failing tests. These are very convenient for us, but there is probably a UX problems for newcomers in that they may not be aware of their existence.
For CI testing, --show-diff is used to dump diffs into the CI log.
For integration with test viewers, JUnit reports are used. This is what we use on Azure, and what some people use with Jenkins, and properly lots of other integrations.

Can you elaborate on where the HTML report fits in, who would be using it, and what improvements you have planned for it?

dragoonis · 2020-05-28T09:31:00Z

Can you elaborate on where the HTML report fits in, who would be using it, and what improvements you have planned for it?

I have to defer beginning this wider conversation due to restrictive time commitments (COVID related).

Things I'll have to do, before I can discuss it: I need to do more research on what other OSS projects are doing, and extract their value and bring it back to PHP. I want to speak to more people about what they'd like to see in it, too.

Thanks

TysonAndre · 2020-05-28T13:08:50Z

run-tests.php

@@ -1688,9 +1698,9 @@ function kill_children(array $children)
    }
 }

-function run_worker()
+function run_worker($workerID)


nit: It seems inconsistent to have this as a global variable in some places but a regular parameter with the same value here

In an effort to move away from the global problem, this is the beginning of
many PRs.

Im trying to keep this PR tiny and focus on HTML bug fix, so i can come
back with another PR with many improvements.

I've started with this small win here, on this line and there will be more
to come.

dragoonis · 2020-05-29T12:19:49Z

@nikic I'm ready to begin the next PR right now. I'm holding off until we resolve this conversation. Can we move it along, please? Thanks

EDIT:
I don't know if we're dropping HTML, or we're keeping it. If we keep it then I'll add PHP7 types. If we drop it then I'll change the code to remove HTML altogether. I'm happy with either outcome, I just need to know what the team think we should do going forward.

derickr · 2020-05-29T12:43:13Z

run-tests.php

@@ -1721,6 +1731,10 @@ function run_worker()
        define($const, $value);
    }

+    if(isset($greeting["output_handler"]['html']['filename'])) {


nit: space between if and (isset

I will format this code properly for styling. (phpStorm does this for me). I will hold off on this as it looks like we're likely to delete this code.

kallesommernielsen · 2020-05-29T13:00:57Z

I think we should just drop the --html option, if the issue first came up now then I think we can easily assume that there is no real users of this and if anything it would simplfying parts of the code by removing it.

Like @nikic said, we only use a handful of formats for third-party CI tooling primarily besides the usual CLI report. We should stick to those.

dragoonis · 2020-05-29T13:24:50Z

Like @nikic said, we only use a handful of formats for third-party CI tooling primarily besides the usual CLI report. We should stick to those.

We are having a discussion on Stack Overflow room now, about who's using it. PhpOnWindows team are using it and @cmb69 is looking into this for us to assess the impact of removing this option.

cmb69 · 2020-06-01T11:56:16Z

PhpOnWindows team are using it […]

I've got confirmation that the --html option is used, but that it's expendable, since the actual .html reports are not used. So feel free to drop the option. :)

dragoonis · 2020-06-09T12:11:48Z

Noted. I'm going to close down this PR, and make a fresh one. Thanks for the input @nikic @TysonAndre @cmb69 @kallesommernielsen #goTeam

dragoonis · 2020-06-13T04:10:23Z

PR closed. Moved work to here: https://github.com/php/php-src/pull/5705/files

As discussed on GH-5632, the HTML functionality does not appear to be in active use. For HTML rendering of test results, it is suggested to instead use the JUnit integration, in combination with your favorite JUnit viewer. Closes GH-5705.

carusogabriel added the Category: Tests label May 27, 2020

nikic reviewed May 27, 2020

View reviewed changes

run-tests.php Outdated Show resolved Hide resolved

Girgias reviewed May 27, 2020

View reviewed changes

Fix #79639 - Preserving the html report options in parallel worker mode

b24051d

dragoonis force-pushed the run-tests-html-fix-79639 branch from 4eb9066 to b24051d Compare May 27, 2020 21:26

TysonAndre reviewed May 28, 2020

View reviewed changes

derickr reviewed May 29, 2020

View reviewed changes

dragoonis closed this Jun 13, 2020

dragoonis mentioned this pull request Jun 13, 2020

Removing HTML Functionality from run-tests.php #5705

Closed

	public function setHtmlOutputFilename($outputFilename)
	public function setHtmlOutputFilename(string $outputFilename)

	public function isHtmlEnabled()
	public function isHtmlEnabled(): bool

	private $htmlOutputFilename = null;
	private ?string $htmlOutputFilename = null;

Fix #79639 - Preserving the html report options in parallel worker mode #5632

Fix #79639 - Preserving the html report options in parallel worker mode #5632

Uh oh!

Conversation

dragoonis commented May 27, 2020

Uh oh!

Uh oh!

nikic commented May 27, 2020

Uh oh!

dragoonis commented May 27, 2020

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dragoonis May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TysonAndre May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic commented May 27, 2020

Uh oh!

dragoonis commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dragoonis commented May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dragoonis May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kallesommernielsen commented May 29, 2020

Uh oh!

dragoonis commented May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmb69 commented Jun 1, 2020

Uh oh!

dragoonis commented Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dragoonis commented Jun 13, 2020

Uh oh!

Uh oh!

dragoonis May 28, 2020 •

edited

Loading

TysonAndre May 28, 2020 •

edited

Loading

dragoonis commented May 28, 2020 •

edited

Loading

dragoonis commented May 29, 2020 •

edited

Loading

dragoonis May 29, 2020 •

edited

Loading

dragoonis commented May 29, 2020 •

edited

Loading

dragoonis commented Jun 9, 2020 •

edited

Loading