-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Provide bless_tests.patch for failing tests #7204
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
Conversation
Apparently, bless_test.php also fixes a few tests which didn't fail according to run-tests.php. Is this a caching issue? I'll have to check. |
What is the best/easiest way to access the artifact? For development, I started using Freebsd/Cirrus CI output as the output is accessible thru single click (like https://github.com/php/php-src/runs/2935436162), however it does not contain a diff. Can the output summary be updated so it contains the diff of |
ext/ffi/tests/bug80847.phpt has an "xfail" SKIPIF on Windows. bless_tests.php ignores XFAIL tests, but not that particular case. |
Right, the other test that is caught under ZTS also. I added a patch for bless_test.php, but I'm not particularly happy with it. |
@cmb69 A regex shows there's only 21 tests that use |
Alternatively we could not emit the |
Actually, I like the distinction between xfail (should work, but does not yet) and skip (can't work), but given that it's rather uncommon that xfail tests get adressed, and that they consume time for no good reason, switching dynamic xfail to skip (plus maybe a comment) might be sensible. |
Might be controversial: Maybe we should not run xfail at all? What's the point if they're gonna fail anyway. If you want to fix them removing the xfail seems sensible. This way we can:
|
@iluuu1994 If we don't run xfail tests, then we'd no longer get the warning if an xfail test actually passes. Not sure how valuable that is though... For the problem here, it might make sense to integrate a |
Yet another option might be to move dynamic xfail to the XFAIL section; empty XFAIL section would be treated like before; non empty XFAIL section would be handled like SKIPIF sections. |
I've added a |
4e145b9
to
722a2ad
Compare
Thank you, @nikic. I've updated the patch accordingly. |
That file is modified before the tests are run, so we need to restore it. It would be better, though, if the file would not need to be modified in the first place.
This reverts commit a5d63f9.
In case any tests are failing, we run bless_tests.php and provide the resulting
git diff
as artifact. This should be helpful for PR authors who don't have a Windows environment at hand.The second commit introduces a test failure to be able to verify the results.