MantisBT

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0001509XdebugCode Coveragepublic2018-01-03 12:562018-01-22 18:21
Reporteremir 
Assigned Toderick 
PrioritynormalSeveritymajorReproducibilityalways
StatusclosedResolutionfixed 
PlatformOSMacOSOS Version10.13.2
Product Version2.6.0beta1 
Target VersionFixed in Version2.6.0rc1 
Summary0001509: Code coverage missing for case inside switch with PHP 7.2
DescriptionCode coverage is not attributed for case statements. This was fixed in https://github.com/xdebug/xdebug/commit/040ec0218d9f07b1d6d0b20dbad0c156e578e6e8 [^] and released in 2.6.0alpha1 but with 2.6.0beta1 it is still not working.
Steps To ReproduceTest:

class DummyTest extends \PHPUnit\Framework\TestCase
{

    public function caseProvider()
    {
        return
        [
            ['foo'],
            ['bar'],
            ['1'],
        ];
    }

    /**
     * @param $case
     * @dataProvider caseProvider
     */
    public function testCase($case)
    {
        $this->assertGreaterThan(0, TestClass::someMethod($case));
    }
}

class TestClass
{
    public static function someMethod($case)
    {
        switch ($case) {
            case 'foo':
                return 1;
            break;
            case 'bar':
                return 2;
                break;
            default:
                return 3;
                break;
        }
    }
}
Additional Information# php -v
PHP 7.2.0 (cli) (built: Dec 3 2017 21:46:44) ( NTS )
Copyright (c) 1997-2017 The PHP Group
Zend Engine v3.2.0, Copyright (c) 1998-2017 Zend Technologies
    with Zend OPcache v7.2.0, Copyright (c) 1999-2017, by Zend Technologies
    with Xdebug v2.6.0beta1, Copyright (c) 2002-2017, by Derick Rethans
TagsNo tags attached.
Operating System
PHP Version7.2.0-7.2.4
Attached Files

- Relationships

-  Notes
(0004541)
derick (administrator)
2018-01-03 20:14
edited on: 2018-01-04 16:19

Hmm, I don't quite know what to do with this. *technically*, this is a "work as designed", as the case statements are never hit, yet they can still be. But I can see why it is confusing.

Look at the VLD dump:

{nowiki}
-------------------------------------------------------------------------------------
   4 0 E > EXT_NOP
         1 RECV !0
   6 2 EXT_STMT
         3 > SWITCH_STRING !0, [ 'foo':->8, 'bar':->10, ], ->12
   7 4 > CASE 0000002:0000001 !0, 'foo'
         5 > JMPNZ 0000002:0000001, ->8
  10 6 > CASE 0000002:0000001 !0, 'bar'
         7 > JMPZNZ 0000002:0000001, ->12, ->10
   8 8 > EXT_STMT
         9 > RETURN 1
  11 10 > EXT_STMT
        11 > RETURN 2
  14 12 > EXT_STMT
        13 > RETURN 3
{nowiki}

There are 4 possible outcomes for the "switch", jump to 8, 10, 12, or 4.

Because you feed it three strings ("foo", "bar", and "1"), the last case (jump to 4), won't be reached. Yet, it is *possible* that it can be. For example, if you feed a 3.14 as argument to the switch. In that case, all the case statements (on line 7 and 10, opcodes 4-7) are correctly covered. But I can understand that *logically*, this can almost never happen.

I don't see a way around this though as ignoring all the case/jmpnz cases isn't proper either and you might get a wrong sense of coverage. If you have an idea, I'd be happy to hear it.

(0004542)
emir (reporter)
2018-01-03 20:25

I am not sure you understood me. This is a clear regression compared to Xdebug 2.5.5 on PHP 7.1.12. There this class gets 100% coverage and the case statements are hit and attributed correctly.
(0004543)
emir (reporter)
2018-01-03 20:26

In PHP 7.2 no case statements are attributed. I have tested this using php-code-coverage 5.3.0 with PHPUnit 6.2.4.
(0004544)
emir (reporter)
2018-01-03 21:07

Here is a shortest way to reproduce it, you can compare 7.1 with Xdebug 2.5.5 vs 7.2 with latest Xdebug dev: https://travis-ci.org/emirb/phpunit-xdebug-coverage-cases [^]
(0004545)
derick (administrator)
2018-01-03 21:34

I understood you perfectly, and I can reproduce it too, but it isn't *wrong* as the case statements itself are never actually hit. They're shortcutted by the switch jump table optimisation, see: https://derickrethans.nl/php7.2-switch.html [^]

In order for the case statements to be covered, you need to feed switch a value that doesn't trigger the jump table optimisation, by giving it a wrong type. If you do that, the case statements are covered.

Xdebug can't know which case statements belong to the entries in the jump table. I also can't simply mark all case statements as "can't reach either", because in some cases they can be.
(0004546)
emir (reporter)
2018-01-03 21:41

I understand, I am aware of switch jump table optimisation but I assumed that its addition will still keep the behaviour the same as it was. The optimisation should benefit the performance only, right?

My assumption (and I guess by majority of other people doing the upgrade )is that the upgrade to 7.2 should *not* decrease the coverage.
Your blog post says that you added support for figuring out the paths to see where jump instructions happened so I expected this to work. Is it really impossible to know which case statements were hit? You already know that the code inside a specific case statement was executed so wouldn't it be trivial to attribute code coverage to the case one layer above?
(0004547)
derick (administrator)
2018-01-04 16:17

I agree that an upgrade to 7.2 shouldn't decrease it, but it unfortunately does.

I can not easily see which specific cases were executed, as they are not stored together in the oparray as should be evident from my first comment.

For example, the statements belonging to "case 'foo'" are done in op codes 8-9, which are not close to either the jump table (op 3), or the case+jump (op 4 and 5). From seeing code executed in op 8-9, I don't know *why* that was.

Perhaps it is possible to see which jump was taken in the switch_string, find out which *other* op codes jump to that line (op 5, potentially), and then scan backwards to the case statement (op 4), but I do not know how reliable this would be. Perhaps Nikita has an idea (I'll email him).

cheers,
Derick
(0004549)
derick (administrator)
2018-01-05 23:59

Cheers, merged into Git master now.

- Issue History
Date Modified Username Field Change
2018-01-03 12:56 emir New Issue
2018-01-03 20:14 derick Note Added: 0004541
2018-01-03 20:14 derick Status new => resolved
2018-01-03 20:14 derick Resolution open => not fixable
2018-01-03 20:14 derick Assigned To => derick
2018-01-03 20:25 emir Note Added: 0004542
2018-01-03 20:25 emir Status resolved => new
2018-01-03 20:25 emir Resolution not fixable => reopened
2018-01-03 20:26 emir Note Added: 0004543
2018-01-03 21:07 emir Note Added: 0004544
2018-01-03 21:34 derick Note Added: 0004545
2018-01-03 21:41 emir Note Added: 0004546
2018-01-04 16:17 derick Note Added: 0004547
2018-01-04 16:18 derick Note Edited: 0004541 View Revisions
2018-01-04 16:18 derick Note Edited: 0004541 View Revisions
2018-01-04 16:19 derick Note Edited: 0004541 View Revisions
2018-01-05 23:59 derick Note Added: 0004549
2018-01-05 23:59 derick Status new => closed
2018-01-05 23:59 derick Resolution reopened => fixed
2018-01-05 23:59 derick Fixed in Version => 2.6.0
2018-01-22 18:21 derick Fixed in Version 2.6.0 => 2.6.0rc1


Copyright © 2000 - 2018 MantisBT Team
Powered by Mantis Bugtracker