Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate and upload JUNIT test results on Windows CI #15897

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Sep 15, 2024

These are useful to get fine grained information about what was going on when the tests have been run, but won't clutter the build logs.


Now I see that these have been deliberately removed via #14555, stating as reasons:

Nobody looks at those, and nightly regularly fails due to uploading them.

The former makes me sad. It's hard to notice that a complete test suite is skipped because a required server isn't properly set up (see e.g. #15896). I would be happy to have the JUNIT results available again, if only for Windows CI.

These are useful to get fine grained information about what was going
on when the tests have been run, but won't clutter the build logs.
@iluuu1994
Copy link
Member

It's hard to notice that a complete test suite is skipped

The issue is that it's hard to notice regardless. Maybe it would be better to specify a list of extensions that are expected to be tested, and error if they aren't.

@cmb69
Copy link
Member Author

cmb69 commented Sep 15, 2024

Maybe it would be better to specify a list of extensions that are expected to be tested, and error if they aren't.

Many (most? all?) tests are skipped if a server is unavailable. These tests would need to be changed. But what should they report? We recently removed a probably unknown environment variable for mysqli tests, which would execute the tests to let them fail (instead of skipping them); that's not really helpful.

Inspecting the junit results manually is tedious (although occassionally useful), but it's easy to write simple scripts like

<?php
$doc = new DomDocument();
$doc->load("D:\Users\cmb\Downloads\junit.out.xml");
$xpath = new DOMXPath($doc);
$res = [];
foreach ($xpath->query("//testsuite[@skip > 10]") as $elt) {
    $perc = $elt->getAttribute("skip") / $elt->getAttribute("tests") * 100;
    if ($perc > 50) {
        $res[$elt->getAttribute("name")] = $perc;
    }
}
arsort($res);
print_r($res);

which outputs:

Array
(
    [php.ext.snmp.tests] => 100
    [php.sapi.fpm.tests] => 100
    [php.ext.pgsql.tests] => 98.913043478261
    [php.ext.ftp.tests] => 98.214285714286
    [php.ext.standard.tests.http] => 96.666666666667
    [php.ext.pdo.tests] => 91.764705882353
)

Okay, snmp is a known issue now (#15896), fpm is non-Windows, pgsql tests are not executed (I know), but I'm surprised about ftp and standard/http. And the pdo tests are probably a false positive (due to redirects). One can have a look into these.

@iluuu1994
Copy link
Member

Many (most? all?) tests are skipped if a server is unavailable.

Which is a little questionable in itself. Does it make sense to have hundreds of tests trying to ping a server that isn't available? IMO, it seems best if these tests didn't have a default configuration in the first place. And when the configuration is provided, not being able to connect should fail the test. Anyway, different issue.

Inspecting the junit results manually is tedious (although occassionally useful), but it's easy to write simple scripts like

It's probably possible to declare some threshold of tests that should be run per extension. E.g. if >50% are skipped, something fishy may be going on. Putting this logic in run-tests.php would make it much more accessible.

@cmb69
Copy link
Member Author

cmb69 commented Sep 16, 2024

Maybe it would be better to specify a list of extensions that are expected to be tested, and error if they aren't.

Maybe something like https://github.com/cmb69/php-ftw/blob/2865a4e059fb0b39f4f0d079c3754092e37f3972/.github/workflows/run-tests.ps1#L55-L61. That doesn't make the tests fail, if they are skipped, but avoids useless tests. Keeping dirs-to-test.txt up-to-date shouldn't be hard, but still is somewhat error prone.

It's probably possible to declare some threshold of tests that should be run per extension. E.g. if >50% are skipped, something fishy may be going on. Putting this logic in run-tests.php would make it much more accessible.

That can make sense. Still, I feel that having the option to look into the junit results can be helpful occassionally ("has a certain test actually be run?"). See e.g. #15361 (comment); I had forced the test to fail, mostly to see that it actually had been executed.

[…], but I'm surprised about ftp and standard/http.

Ah, nope, known issue. The servers are using pcntl_fork() which is not available on Windows. So the solution is simple: just port fork() to Windows. ;)

@iluuu1994
Copy link
Member

Just to clarify: I don't object to re-adding JUNIT. I didn't find it useful, and time-consuming to look at. I would prefer if we could also generate some more useful statistics though, that don't require downloading and scanning a huge file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants