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

Fix for declaring test case in data provider #2861

Merged
merged 6 commits into from Nov 27, 2017
Merged

Fix for declaring test case in data provider #2861

merged 6 commits into from Nov 27, 2017

Conversation

kubawerlos
Copy link
Contributor

For #2833.

I cannot reproduce the problem from #2859 - as I suspected the minimal, self-contained, reproducing test case from #2859 (comment) is not triggering the error. Obviously I have tested my original solution in project with over dozen of data providers and all went fine.

@sebastianbergmann can you create a branch for the issue and merge this PR on it so @eddmann, @moorscode or @johnbillion will provide a test case for the problem?

@kubawerlos kubawerlos changed the title Bugfix/fixing class declaration in data provider attempt 2 Fix for declaring test case in data provider Nov 14, 2017
@codecov-io
Copy link

codecov-io commented Nov 14, 2017

Codecov Report

Merging #2861 into 5.7 will increase coverage by <.01%.
The diff coverage is 80%.

Impacted file tree graph

@@             Coverage Diff              @@
##                5.7    #2861      +/-   ##
============================================
+ Coverage     51.18%   51.18%   +<.01%     
- Complexity     2912     2913       +1     
============================================
  Files           110      110              
  Lines          9706     9709       +3     
============================================
+ Hits           4968     4970       +2     
- Misses         4738     4739       +1
Impacted Files Coverage Δ Complexity Δ
src/Framework/TestSuite.php 68.63% <80%> (-0.02%) 150 <0> (+1)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb52087...5323e21. Read the comment docs.

{
const DUMMY = 'dummy';

public function testSecond()
Copy link

@gavinlove gavinlove Nov 15, 2017

Choose a reason for hiding this comment

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

Change this to use a data provider and it is likely you will see the problem.

@kubawerlos
Copy link
Contributor Author

@gavinlove like this dcace3b ? Of course I tried something like this yesterday, give me some credit - I tried to follow instructions from the #2859 - none of them work. More, few minutes ago I have run PHPUnit 5.7.24 on master of PHP-CS-Fixer which have 495 data providers an (with PHP 7.1.11-1+ubuntu16.04.1+deb.sury.org+1) - all went fine.

TBH it feels little bit irritated - I have discovered the bug - this one that test class loaded during data provider execution is not discovered - provided decent test case to cover the problem and then made a fix. I have tried the fix with multiple projects I have on my PC and had no error. I'd say I proceeded with the bug quite well.

Then #2859 was reported with non-working reproductions steps (few snippets, none of them leading to the error). Or maybe I'm wrong here, @sebastianbergmann did you manage to reproduce it, so we can create PHPT to cover it? If not I'd like to fix for this issue be accepted and merged and #2859 be fixed not by reverting fix of the other bug, but in proper way - PHPT case and then fix (which BTW I'd be happy to implement).

@gavinlove
Copy link

gavinlove commented Nov 15, 2017

@kubawerlos Your right the two providers is a not the right way to reproduce it. I looked at what @eddmann said I was able to reproduce this.

I created a new directory and added a new composer.json

{
    "name": "foo/data-provider-declaring-class",
    "version": "0.0.1",
    "require": {
        "php": ">=7.1"
    },
    "require-dev": {
        "phpunit/phpunit": "5.7.24"
    },
    "autoload": {
        "psr-4": {
            "Foo\\DataProviderDeclaringClass\\": "src/"
        }
    },
    "config": {
        "bin-dir": "bin"
    }
}

I used your FirstTest class which I placed in a tests/another directory.

Then I added a phpunit.xml.dist with the following

<?xml version="1.0" encoding="UTF-8"?>

<phpunit bootstrap="vendor/autoload.php">
    <testsuites>
        <testsuite name="Test Suite">
            <directory>./tests/</directory>
            <directory>./tests/*/</directory>
        </testsuite>
    </testsuites>
</phpunit>

Note that the same directory is listed twice here which @eddmann mentioned yesterday.

I then ran bin/phpunit and got the same error.

I suspect what is happening is that the directory is parsed twice and then later on phpunit filters it down to a single test.

@johnbillion
Copy link
Sponsor Contributor

Please don't be irritated! We're simply reporting what we're seeing.

AFAICT, the minimal test case I reported in #2859 is enough to reproduce the issue, but there must be something else which causes this, but I don't know what.

I don't think the issue is dependent on PHP version. The issue appears for me when running PHP 7.1 and 7.2.

If it helps, here's a Travis CI job that failed due to this fatal error: https://travis-ci.org/johnbillion/query-monitor/jobs/301942375 .

@gavinlove
Copy link

@kubawerlos @johnbillion I have pushed up a repo which reproduces it for me, which will hopefully help.

https://github.com/gavinlove/phpunit-bug-2859

@kubawerlos
Copy link
Contributor Author

@gavinlove thanks, your first comment gave me a hint and I just uploaded test case for the problem. Fix in progress.

@kubawerlos
Copy link
Contributor Author

@sebastianbergmann fix might looks as really dummy, but in fact PHPUnit_Framework_TestSuite_DataProvider is the only PHPUnit class that implements PHPUnit_Framework_Test and would wrongly go into the calling of addTestSuite 16 lines below the added if.

@@ -87,6 +87,11 @@ class PHPUnit_Framework_TestSuite implements PHPUnit_Framework_Test, PHPUnit_Fra
*/
private $iteratorFilter = null;

/**
* @var array
Copy link
Contributor

Choose a reason for hiding this comment

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

array of ? strings? then string[]

@sebastianbergmann sebastianbergmann merged commit ad12c6a into sebastianbergmann:5.7 Nov 27, 2017
SimonHeimberg added a commit to SimonHeimberg/phpunit that referenced this pull request Dec 5, 2017
@kubawerlos kubawerlos deleted the bugfix/fixing-class-declaration-in-data-provider-attempt-2 branch December 6, 2017 17:20
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

6 participants