Skip to content

Fix ICU dependant tests #21320

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 2 commits into from
Jan 24, 2017
Merged

Fix ICU dependant tests #21320

merged 2 commits into from
Jan 24, 2017

Conversation

jakzal
Copy link
Contributor

@jakzal jakzal commented Jan 17, 2017

Q A
Branch? 2.7
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

#20551 changed the condition checking if the ICU version matches the stubbed data. The change intended to enable tests on more ICU versions, but it actually has limited them. I'm still not convinced it should've been done but let's at least fix the condition. Ideal solution would be to have the latest ICU data available on travis (still not there travis-ci/travis-ci#3616).

I also needed to fix several tests.

Currently skipped tests in components depending on ICU data:

4.8.1.1 54.1 55.1 57.1
Intl 488 488 7 6
Locale 0 0 0 0
Translation 0 0 0 0
Validator 69 69 69 0
Form 75 75 75 1

@jakzal jakzal added the Intl label Jan 17, 2017
@javiereguiluz javiereguiluz changed the title Fix ICU depndant tests Fix ICU dependant tests Jan 17, 2017
@@ -108,10 +108,10 @@ public function testReverseTransformEmpty()

public function testReverseTransformWithGrouping()
{
// Since we test against "de_AT", we need the full implementation
// Since we test against "de_DE", we need the full implementation
Copy link
Contributor Author

Choose a reason for hiding this comment

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

German data seems to be changing less often.

@stof
Copy link
Member

stof commented Jan 17, 2017

Currently skipped tests in components depending on ICU data:

is this table built before or after your change ?

@jakzal
Copy link
Contributor Author

jakzal commented Jan 17, 2017

@stof after my change. Here's the current 2.7:

4.8.1.1 54.1 55.1 57.1
Intl 488 488 7 488
Locale 0 0 0 0
Translation 0 0 0 0
Validator 69 69 69 0
Form 75 75 + 1 error + 5 failures 75 1 + 3 errors + 3 failures

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Jan 24, 2017
@nicolas-grekas
Copy link
Member

👍

@fabpot
Copy link
Member

fabpot commented Jan 24, 2017

Thank you @jakzal.

@fabpot fabpot merged commit d3b5d8e into symfony:2.7 Jan 24, 2017
fabpot added a commit that referenced this pull request Jan 24, 2017
This PR was squashed before being merged into the 2.7 branch (closes #21320).

Discussion
----------

Fix ICU dependant tests

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

#20551 changed the condition checking if the ICU version matches the stubbed data. The change intended to enable tests on more ICU versions, but it actually has limited them. I'm still not convinced it should've been done but let's at least fix the condition. Ideal solution would be to have the latest ICU data available on travis (still not there travis-ci/travis-ci#3616).

I also needed to fix several tests.

Currently skipped tests in components depending on ICU data:

|            | 4.8.1.1 | 54.1 | 55.1 | 57.1 |
|---|---|--|--|--|
| Intl       | 488     | 488  | 7    | 6    |
| Locale     | 0       | 0    | 0    | 0    |
| Translation| 0       | 0    | 0    | 0    |
| Validator  | 69      | 69   | 69   | 0    |
| Form       | 75      | 75   | 75   | 1    |

Commits
-------

d3b5d8e Fix tests with ICU 57.1
677d820 Fix the condition checking the minimum ICU version
@jakzal jakzal deleted the icu-tests-fix branch January 24, 2017 16:02
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.

5 participants