Skip to content

Fix ICU version specific skip reasons of intl tests #16661

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

Merged
merged 1 commit into from
Nov 2, 2024

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Oct 31, 2024

  • Some tests have this backwards, probably reading "skip for ICU …". However, that is not how skip reasons are handled.
  • Some test seems to have typos in the skip reason version.
  • Some tests checked for a certain version, but reported the presumably next version, which is confusing at best.
  • Some tests checked versions in descending order, what is not wrong, but confusing.
  • Some tests had off by one errors.

For all tests, we assume that the skipif conditions are correct, and fix the reasons.


Note that I made this in preparation for #16659, to not completely loose my sanity.

* Some tests have this backwards, probably reading "skip for ICU …".
  However, that is not how skip reasons are handled.
* Some test seems to have typos in the skip reason version.
* Some tests checked for a certain version, but reported the
  *presumably* next version, which is confusing at best.
* Some tests checked versions in descending order, what is not wrong,
  but confusing.
* Some tests had off by one errors.

For all tests, we assume that the skipif conditions are correct, and
fix the reasons.
Copy link
Member

@devnexen devnexen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks !

@cmb69 cmb69 merged commit 9afc66f into php:master Nov 2, 2024
8 of 10 checks passed
@cmb69 cmb69 deleted the cmb/icu-versions branch November 2, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants