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

Fix test suite when ext/intl isn't available #5111

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@dshafik
Contributor

dshafik commented Sep 14, 2013

  • This will hopefully enable the testsuite to run in HHVM (see: this
    blog post
    )
  • There were a bunch of errors caused by Zend\I18n\Validator\Int,
    switching some tests to use Zend\Validation\Digits might be a good
    idea
@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Sep 14, 2013

Member

did you look into #5110 first? :)

Member

Ocramius commented Sep 14, 2013

did you look into #5110 first? :)

@dshafik

This comment has been minimized.

Show comment
Hide comment
@dshafik

dshafik Sep 14, 2013

Contributor

@Ocramius of... course not. BLAH!

I'll work with @TheFrozenFire to see which works better :)

Contributor

dshafik commented Sep 14, 2013

@Ocramius of... course not. BLAH!

I'll work with @TheFrozenFire to see which works better :)

/**
* @category Zend
* @package Zend_Form
* @subpackage UnitTest

This comment has been minimized.

@samsonasik

samsonasik Sep 14, 2013

Contributor

remove @category @Package and @subpackage

@samsonasik

samsonasik Sep 14, 2013

Contributor

remove @category @Package and @subpackage

This comment has been minimized.

@dshafik

dshafik Sep 15, 2013

Contributor

I'm not sure why you are wanting to do this? I followed the convention in the other tests.

@dshafik

dshafik Sep 15, 2013

Contributor

I'm not sure why you are wanting to do this? I followed the convention in the other tests.

This comment has been minimized.

@dshafik

dshafik Sep 15, 2013

Contributor

#3508 only applies to library and explicitly says not to apply to tests.

@dshafik

dshafik Sep 15, 2013

Contributor

#3508 only applies to library and explicitly says not to apply to tests.

This comment has been minimized.

@samsonasik

samsonasik Sep 15, 2013

Contributor

please take a look #3508 , it's old issue that we agree to remove all @category @Package and @subpackage. I only comment to remove new created one/all of them.

@samsonasik

samsonasik Sep 15, 2013

Contributor

please take a look #3508 , it's old issue that we agree to remove all @category @Package and @subpackage. I only comment to remove new created one/all of them.

* @link http://github.com/zendframework/zf2 for the canonical source repository
* @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
* @license http://framework.zend.com/license/new-bsd New BSD License
* @package Zend_Form

This comment has been minimized.

@samsonasik

samsonasik Sep 14, 2013

Contributor

remove @Package

@samsonasik

This comment has been minimized.

@dshafik

dshafik Sep 15, 2013

Contributor

Same as above: what's the reasoning?

@dshafik

dshafik Sep 15, 2013

Contributor

Same as above: what's the reasoning?

This comment has been minimized.

@samsonasik

samsonasik Sep 15, 2013

Contributor

please take a look #3508 , it's old issue that we agree to remove all @category @Package and @subpackage annotations.
ping @weierophinney

@samsonasik

samsonasik Sep 15, 2013

Contributor

please take a look #3508 , it's old issue that we agree to remove all @category @Package and @subpackage annotations.
ping @weierophinney

This comment has been minimized.

@dshafik

dshafik Sep 15, 2013

Contributor

#3508 only applies to library and explicitly says not to apply to tests.

@dshafik

dshafik Sep 15, 2013

Contributor

#3508 only applies to library and explicitly says not to apply to tests.

This comment has been minimized.

@samsonasik

samsonasik Sep 15, 2013

Contributor

ah, yes. Library only. but I think we need to prevent new created one :)

@samsonasik

samsonasik Sep 15, 2013

Contributor

ah, yes. Library only. but I think we need to prevent new created one :)

This comment has been minimized.

@samsonasik

samsonasik Oct 2, 2013

Contributor

ping @Maks3w

@samsonasik

This comment has been minimized.

@samsonasik

samsonasik Oct 3, 2013

Contributor

@Maks3w I can't push someone that decline to remove @Package docblock, maybe you can ? :)

@samsonasik

samsonasik Oct 3, 2013

Contributor

@Maks3w I can't push someone that decline to remove @Package docblock, maybe you can ? :)

This comment has been minimized.

@Maks3w

Maks3w Oct 3, 2013

Member

@dshafik that docblock tags are deprecated if you use namespaces since the namespace reflects the same info about package categorization.

@Maks3w

Maks3w Oct 3, 2013

Member

@dshafik that docblock tags are deprecated if you use namespaces since the namespace reflects the same info about package categorization.

@dshafik

This comment has been minimized.

Show comment
Hide comment
@dshafik

dshafik Sep 15, 2013

Contributor

This latest change takes sebastianbergmann/phpunit#997 into account — it's conditional on PHPUnit 3.8 (unreleased) and works with < PHPUnit 3.8 as well for now. Don't let it stop this going in. If @sebastianbergmann rejects the PR I'll patch these tests to remove it.

Contributor

dshafik commented Sep 15, 2013

This latest change takes sebastianbergmann/phpunit#997 into account — it's conditional on PHPUnit 3.8 (unreleased) and works with < PHPUnit 3.8 as well for now. Don't let it stop this going in. If @sebastianbergmann rejects the PR I'll patch these tests to remove it.

use PHPUnit_Framework_TestCase as TestCase;
use Zend\Form\Element\DateSelect;
use Zend\Form\View\Helper\FormDateSelect as FormDateSelectHelper;

This comment has been minimized.

@Maks3w

Maks3w Oct 21, 2013

Member

Following framework practice please split this test file and add a new one for each helper

@Maks3w

Maks3w Oct 21, 2013

Member

Following framework practice please split this test file and add a new one for each helper

{
if (!extension_loaded('intl')) {

This comment has been minimized.

@Maks3w

Maks3w Oct 21, 2013

Member

Please remove this from the dataprovider and add it in the test method

@Maks3w

Maks3w Oct 21, 2013

Member

Please remove this from the dataprovider and add it in the test method

This comment has been minimized.

@dshafik

dshafik Oct 21, 2013

Contributor

This is actually required for this to work. The data providers are called BEFORE the test and use ext/intl to generate the test data. If you remove this, the tests will fatal error without ext/intl enabled.

I will not be changing this.

@dshafik

dshafik Oct 21, 2013

Contributor

This is actually required for this to work. The data providers are called BEFORE the test and use ext/intl to generate the test data. If you remove this, the tests will fatal error without ext/intl enabled.

I will not be changing this.

This comment has been minimized.

@Maks3w

Maks3w Oct 21, 2013

Member

Instead return an empty array.

@Maks3w

Maks3w Oct 21, 2013

Member

Instead return an empty array.

This comment has been minimized.

@dshafik

dshafik Oct 21, 2013

Contributor

On what condition would I return this array? Your comment makes no sense and doesn't help solve the issue.

You'll notice I do return an empty nested array (otherwise it's an error, instead of a warning) 4 lines down, unless you have a newer version of phpunit which allows you to markTestSkipped in the provider.

@dshafik

dshafik Oct 21, 2013

Contributor

On what condition would I return this array? Your comment makes no sense and doesn't help solve the issue.

You'll notice I do return an empty nested array (otherwise it's an error, instead of a warning) 4 lines down, unless you have a newer version of phpunit which allows you to markTestSkipped in the provider.

This comment has been minimized.

@Maks3w

Maks3w Oct 21, 2013

Member

@dshafik There is no option for add references to non existing PHPUnit versions. So don't add this kind of code.

@Maks3w

Maks3w Oct 21, 2013

Member

@dshafik There is no option for add references to non existing PHPUnit versions. So don't add this kind of code.

This comment has been minimized.

@dshafik

dshafik Oct 21, 2013

Contributor

OK, last try:

The data provider requires ext/intl. The test also requires ext/intl. If ext/intl doesn't exist, it would be correct to skip the test (markTestSkipped). However, the data provider is called before the test (which makes sense, as it gives the arguments for the test method), so it fatal errors before it hits the markTestSkipped.

The obvious solution therefore, is to markTestSkipped in the data provider. In the current PHPUnit (3.7) this is not possible. So you instead return an empty data set.

If you return an empty data set (array(array())), phpunit will emit a warning, and the test suite may be considered failed (e.g. continuous integration that blocks on errors/warnings).

So, I added a PR for PHPUnit to add the ability to markTestSkipped in the provider.

This code in the tests will stop the fatal errors in PHPUnit 3.7 (by checking for ext/intl and returning an empty data set when it doesn't exist) , and in newer versions of PHPUnit (> 3.7, e.g. 3.8-dev) will stop the warnings (and skip the test correctly).

I did not add this just for funsies, I added it for well thought out reasons. If you do not want to merge these changes (after I remove the @Package tags) then just close the PR and reject it.

@dshafik

dshafik Oct 21, 2013

Contributor

OK, last try:

The data provider requires ext/intl. The test also requires ext/intl. If ext/intl doesn't exist, it would be correct to skip the test (markTestSkipped). However, the data provider is called before the test (which makes sense, as it gives the arguments for the test method), so it fatal errors before it hits the markTestSkipped.

The obvious solution therefore, is to markTestSkipped in the data provider. In the current PHPUnit (3.7) this is not possible. So you instead return an empty data set.

If you return an empty data set (array(array())), phpunit will emit a warning, and the test suite may be considered failed (e.g. continuous integration that blocks on errors/warnings).

So, I added a PR for PHPUnit to add the ability to markTestSkipped in the provider.

This code in the tests will stop the fatal errors in PHPUnit 3.7 (by checking for ext/intl and returning an empty data set when it doesn't exist) , and in newer versions of PHPUnit (> 3.7, e.g. 3.8-dev) will stop the warnings (and skip the test correctly).

I did not add this just for funsies, I added it for well thought out reasons. If you do not want to merge these changes (after I remove the @Package tags) then just close the PR and reject it.

This comment has been minimized.

@jmather

jmather Oct 21, 2013

What if instead the test that was dependent on php/intl was moved to a separate php/intl test suite, so that only the test in question is skipped. Is that the core of the issue, @Maks3w? Other tests being skipped? If so, this one just needs to be silo'd in it's own test suite so it can be skipped properly.

@jmather

jmather Oct 21, 2013

What if instead the test that was dependent on php/intl was moved to a separate php/intl test suite, so that only the test in question is skipped. Is that the core of the issue, @Maks3w? Other tests being skipped? If so, this one just needs to be silo'd in it's own test suite so it can be skipped properly.

This comment has been minimized.

@weierophinney

weierophinney Oct 22, 2013

Member

@Maks3w I'm okay with it as written, as it allows changing the behavior based on PHPUnit version. The logic here is sound.

@weierophinney

weierophinney Oct 22, 2013

Member

@Maks3w I'm okay with it as written, as it allows changing the behavior based on PHPUnit version. The logic here is sound.

@@ -91,8 +106,16 @@ public static function numberToFormattedProvider()
);
}
public static function formattedToNumberProvider()
public function formattedToNumberProvider()

This comment has been minimized.

@Maks3w

Maks3w Oct 21, 2013

Member

here too

@Maks3w

Maks3w Oct 21, 2013

Member

here too

This comment has been minimized.

@dshafik

dshafik Oct 21, 2013

Contributor

Same here

@dshafik

dshafik Oct 21, 2013

Contributor

Same here

@@ -65,6 +71,14 @@ public function testBasic($value, $expected, $options = array())
public function basicProvider()
{
if (!extension_loaded('intl')) {
if (version_compare(\PHPUnit_Runner_Version::VERSION, '3.8.0-dev') === 1) {

This comment has been minimized.

@Maks3w

Maks3w Oct 21, 2013

Member

here too

@Maks3w

Maks3w Oct 21, 2013

Member

here too

This comment has been minimized.

@dshafik

dshafik Oct 21, 2013

Contributor

Same here

@dshafik

dshafik Oct 21, 2013

Contributor

Same here

@@ -55,6 +59,14 @@ public function tearDown()
public function dateTestsDataProvider()
{
if (!extension_loaded('intl')) {
if (version_compare(\PHPUnit_Runner_Version::VERSION, '3.8.0-dev') === 1) {

This comment has been minimized.

@Maks3w

Maks3w Oct 21, 2013

Member

here too

@Maks3w

Maks3w Oct 21, 2013

Member

here too

This comment has been minimized.

@dshafik

dshafik Oct 21, 2013

Contributor

Same here

@dshafik

dshafik Oct 21, 2013

Contributor

Same here

@@ -147,6 +159,14 @@ public function dateTestsDataProvider()
public function dateTestsDataProviderWithPattern()
{
if (!extension_loaded('intl')) {

This comment has been minimized.

@Maks3w

Maks3w Oct 21, 2013

Member

here too

@Maks3w

Maks3w Oct 21, 2013

Member

here too

This comment has been minimized.

@dshafik

dshafik Oct 21, 2013

Contributor

Same here

@dshafik

dshafik Oct 21, 2013

Contributor

Same here

@@ -54,6 +58,14 @@ public function tearDown()
public function currencyTestsDataProvider()
{
if (!extension_loaded('intl')) {
if (version_compare(\PHPUnit_Runner_Version::VERSION, '3.8.0-dev') === 1) {

This comment has been minimized.

@Maks3w

Maks3w Oct 21, 2013

Member

here too

@Maks3w

Maks3w Oct 21, 2013

Member

here too

This comment has been minimized.

@dshafik

dshafik Oct 21, 2013

Contributor

Same here

@dshafik

dshafik Oct 21, 2013

Contributor

Same here

@Maks3w

This comment has been minimized.

Show comment
Hide comment
@Maks3w

Maks3w Oct 21, 2013

Member

@dshafik ZF2 has a minimum requirement on PHPUnit 3.7 so why not use @requires annotation?

http://phpunit.de/manual/3.7/en/incomplete-and-skipped-tests.html#incomplete-and-skipped-tests.requires.tables.api

Member

Maks3w commented Oct 21, 2013

@dshafik ZF2 has a minimum requirement on PHPUnit 3.7 so why not use @requires annotation?

http://phpunit.de/manual/3.7/en/incomplete-and-skipped-tests.html#incomplete-and-skipped-tests.requires.tables.api

@dshafik

This comment has been minimized.

Show comment
Hide comment
@dshafik

dshafik Oct 21, 2013

Contributor

@Maks3w I believe it's because the data provider is still called. I don't remember why they didn't work, but I did try this and other options first.

Contributor

dshafik commented Oct 21, 2013

@Maks3w I believe it's because the data provider is still called. I don't remember why they didn't work, but I did try this and other options first.

@Maks3w

This comment has been minimized.

Show comment
Hide comment
@Maks3w

Maks3w Oct 21, 2013

Member

@dshafik You'll get a better success in your PHPUnit PR if you prevent trigger @dataProvider when @requires is false

Member

Maks3w commented Oct 21, 2013

@dshafik You'll get a better success in your PHPUnit PR if you prevent trigger @dataProvider when @requires is false

@dshafik

This comment has been minimized.

Show comment
Hide comment
@dshafik

dshafik Oct 21, 2013

Contributor

@Maks3w then it would still run in older versions and fatal error. This has to be something that is possible to work in current and newer versions of PHPUnit. Fixing the @dataProvider/@requires thing for > 3.7 doesn't help, this code would STILL need to be in the dataProvider (at least the intl check and return empty data set).

Contributor

dshafik commented Oct 21, 2013

@Maks3w then it would still run in older versions and fatal error. This has to be something that is possible to work in current and newer versions of PHPUnit. Fixing the @dataProvider/@requires thing for > 3.7 doesn't help, this code would STILL need to be in the dataProvider (at least the intl check and return empty data set).

weierophinney added a commit that referenced this pull request Oct 22, 2013

Merge pull request #5111 from dshafik/fix/ext-intl-missing-failure
Fix test suite when ext/intl isn't available

weierophinney added a commit that referenced this pull request Oct 22, 2013

[#5111] CS fixes
- EOF ending
- Remove unnecessary annotations

weierophinney added a commit that referenced this pull request Oct 22, 2013

Merge branch 'hotfix/5111' into develop
Forward port #5111

Conflicts:
	tests/ZendTest/I18n/Translator/Loader/GettextTest.php
	tests/ZendTest/I18n/Translator/Loader/PhpArrayTest.php

@ghost ghost assigned weierophinney Oct 22, 2013

@Maks3w

This comment has been minimized.

Show comment
Hide comment
@Maks3w

Maks3w Oct 22, 2013

Member

@weierophinney The referenced PHPUnit versions DON'T EXISTS! The patch proposed is not merged.

Member

Maks3w commented Oct 22, 2013

@weierophinney The referenced PHPUnit versions DON'T EXISTS! The patch proposed is not merged.

@weierophinney

This comment has been minimized.

Show comment
Hide comment
@weierophinney

weierophinney Oct 23, 2013

Member

@Maks3w If Sebastian chooses not to merge it, @dshafik has indicated he'll issue a revert. Indications at this point are that it will be merged, however.

Member

weierophinney commented Oct 23, 2013

@Maks3w If Sebastian chooses not to merge it, @dshafik has indicated he'll issue a revert. Indications at this point are that it will be merged, however.

weierophinney added a commit to zendframework/zend-config that referenced this pull request May 15, 2015

Merge pull request zendframework/zendframework#5111 from dshafik/fix/…
…ext-intl-missing-failure

Fix test suite when ext/intl isn't available

weierophinney added a commit to zendframework/zend-config that referenced this pull request May 15, 2015

weierophinney added a commit to zendframework/zend-config that referenced this pull request May 15, 2015

Merge branch 'hotfix/5111' into develop
Forward port zendframework/zendframework#5111

Conflicts:
	tests/ZendTest/I18n/Translator/Loader/GettextTest.php
	tests/ZendTest/I18n/Translator/Loader/PhpArrayTest.php

weierophinney added a commit to zendframework/zend-i18n that referenced this pull request May 15, 2015

Merge pull request zendframework/zendframework#5111 from dshafik/fix/…
…ext-intl-missing-failure

Fix test suite when ext/intl isn't available

weierophinney added a commit to zendframework/zend-i18n that referenced this pull request May 15, 2015

weierophinney added a commit to zendframework/zend-i18n that referenced this pull request May 15, 2015

Merge branch 'hotfix/5111' into develop
Forward port zendframework/zendframework#5111

Conflicts:
	tests/ZendTest/I18n/Translator/Loader/GettextTest.php
	tests/ZendTest/I18n/Translator/Loader/PhpArrayTest.php

weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015

Merge pull request zendframework/zendframework#5111 from dshafik/fix/…
…ext-intl-missing-failure

Fix test suite when ext/intl isn't available

weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015

weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015

Merge branch 'hotfix/5111' into develop
Forward port zendframework/zendframework#5111

Conflicts:
	tests/ZendTest/I18n/Translator/Loader/GettextTest.php
	tests/ZendTest/I18n/Translator/Loader/PhpArrayTest.php

weierophinney added a commit to zendframework/zend-filter that referenced this pull request May 15, 2015

Merge pull request zendframework/zendframework#5111 from dshafik/fix/…
…ext-intl-missing-failure

Fix test suite when ext/intl isn't available

weierophinney added a commit to zendframework/zend-filter that referenced this pull request May 15, 2015

weierophinney added a commit to zendframework/zend-filter that referenced this pull request May 15, 2015

Merge branch 'hotfix/5111' into develop
Forward port zendframework/zendframework#5111

Conflicts:
	tests/ZendTest/I18n/Translator/Loader/GettextTest.php
	tests/ZendTest/I18n/Translator/Loader/PhpArrayTest.php

weierophinney added a commit to zendframework/zend-validator that referenced this pull request May 15, 2015

Merge pull request zendframework/zendframework#5111 from dshafik/fix/…
…ext-intl-missing-failure

Fix test suite when ext/intl isn't available

weierophinney added a commit to zendframework/zend-validator that referenced this pull request May 15, 2015

weierophinney added a commit to zendframework/zend-validator that referenced this pull request May 15, 2015

Merge branch 'hotfix/5111' into develop
Forward port zendframework/zendframework#5111

Conflicts:
	tests/ZendTest/I18n/Translator/Loader/GettextTest.php
	tests/ZendTest/I18n/Translator/Loader/PhpArrayTest.php

weierophinney added a commit to zendframework/zend-log that referenced this pull request May 15, 2015

Merge pull request zendframework/zendframework#5111 from dshafik/fix/…
…ext-intl-missing-failure

Fix test suite when ext/intl isn't available

weierophinney added a commit to zendframework/zend-log that referenced this pull request May 15, 2015

weierophinney added a commit to zendframework/zend-log that referenced this pull request May 15, 2015

Merge branch 'hotfix/5111' into develop
Forward port zendframework/zendframework#5111

Conflicts:
	tests/ZendTest/I18n/Translator/Loader/GettextTest.php
	tests/ZendTest/I18n/Translator/Loader/PhpArrayTest.php

weierophinney added a commit to zendframework/zend-view that referenced this pull request May 15, 2015

Merge pull request zendframework/zendframework#5111 from dshafik/fix/…
…ext-intl-missing-failure

Fix test suite when ext/intl isn't available

weierophinney added a commit to zendframework/zend-view that referenced this pull request May 15, 2015

weierophinney added a commit to zendframework/zend-view that referenced this pull request May 15, 2015

Merge branch 'hotfix/5111' into develop
Forward port zendframework/zendframework#5111

Conflicts:
	tests/ZendTest/I18n/Translator/Loader/GettextTest.php
	tests/ZendTest/I18n/Translator/Loader/PhpArrayTest.php
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment