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

Provide bless_tests.patch for failing tests #7204

Closed
wants to merge 10 commits into from

Conversation

cmb69
Copy link
Contributor

@cmb69 cmb69 commented Jun 28, 2021

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.

@cmb69
Copy link
Contributor Author

cmb69 commented Jun 28, 2021

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.

@mvorisek
Copy link
Contributor

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 FAILED TEST SUMMARY jobs?

@nikic
Copy link
Member

nikic commented Jun 28, 2021

ext/ffi/tests/bug80847.phpt has an "xfail" SKIPIF on Windows. bless_tests.php ignores XFAIL tests, but not that particular case.

@cmb69
Copy link
Contributor Author

cmb69 commented Jun 28, 2021

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.

@iluuu1994
Copy link
Member

@cmb69 A regex shows there's only 21 tests that use xfail in SKIPIF. Should we just convert them to skip to avoid the inaccurate regex?

@iluuu1994
Copy link
Member

iluuu1994 commented Jun 28, 2021

Alternatively we could not emit the .out file for xfail in CI via some flag. Not sure if that's better. bless shouldn't fix xfail tests locally either.

@cmb69
Copy link
Contributor Author

cmb69 commented Jun 28, 2021

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.

@iluuu1994
Copy link
Member

iluuu1994 commented Jul 1, 2021

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:

  • Keep the distinction of skip and xfail for developers
  • Reduce unnecessary time running the tests
  • Fix the bless problem

@nikic
Copy link
Member

nikic commented Jul 5, 2021

@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 --bless option or so in run-tests.php, which can automatically bless failed tests (and has the necessary knowledge about xfailed tests).

@cmb69
Copy link
Contributor Author

cmb69 commented Jul 9, 2021

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.

@nikic
Copy link
Member

nikic commented Jul 16, 2021

I've added a --bless option in bf40a93.

@cmb69
Copy link
Contributor Author

cmb69 commented Jul 17, 2021

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.
@cmb69 cmb69 marked this pull request as ready for review July 17, 2021 14:19
@cmb69 cmb69 closed this in dff3219 Jul 17, 2021
@cmb69 cmb69 deleted the cmb/bless-tests-results branch July 17, 2021 14:58
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.

None yet

4 participants