Skip to content
This repository has been archived by the owner on Mar 30, 2018. It is now read-only.

Mutation for functions #194

Closed
theofidry opened this issue Oct 28, 2016 · 16 comments
Closed

Mutation for functions #194

theofidry opened this issue Oct 28, 2016 · 16 comments
Labels

Comments

@theofidry
Copy link
Member

I have the following piece of code in my codebase:

// src/helpers.php

if (false === function_exists('deep_clone')) {
    /**
     * Deep clone the given value.
     *
     * @param mixed $value
     *
     * @return mixed
     */
    function deep_clone($value)
    {
        return (new \DeepCopy\DeepCopy())->copy($value);
    }
}

And I have a test for it. Humbug mutate this function into:

if (false === function_exists('deep_clone')) {
    /**
     * Deep clone the given value.
     *
     * @param mixed $value
     *
     * @return mixed
     */
    function deep_clone($value)
    {
        (new \DeepCopy\DeepCopy())->copy($value); return null;
    }
}

Which should fail (if I change my source code into that, it does fail) but is not.

@villfa
Copy link
Contributor

villfa commented Nov 4, 2016

Hi,

Out of curiosity I tried to reproduce your problem but it works fine with me (and Humbug mutates the function the same way).

Here my test :

    public function test_deep_clone()
    {
        $o1 = (object) [];
        $o1->foo = 'bar';

        $o2 = deep_clone($o1);

        self::assertEquals('bar', $o2->foo);
    }

and the result I got from humbug :

Humbug version 1.0-dev

Humbug running test suite to generate logs and code coverage data...

    1 [==========================================================] < 1 sec

Humbug has completed the initial test run successfully.
Tests: 1 Line Coverage: 100.00%

Humbug is analysing source files...

Mutation Testing is commencing on 1 files...
(.: killed, M: escaped, S: uncovered, E: fatal error, T: timed out)

.

1 mutations were generated:
       1 mutants were killed
       0 mutants were not covered by tests
       0 covered mutants were not detected
       0 fatal errors were encountered
       0 time outs were encountered

Metrics:
    Mutation Score Indicator (MSI): 100%
    Mutation Code Coverage: 100%
    Covered Code MSI: 100%

I could investigate more if you'd share your test.

@MarkRedeman
Copy link
Contributor

The problem is that in your example you have a method inside of a class
whereas Theo has a function defined in some file. Humbug only mutates
methods and will skip mutations produced outside of a method body. (See
Mutable.php
https://github.com/padraic/humbug/blob/master/src/Mutable.php#L161)

I'm not sure if this check is still needed though.

On Fri, 4 Nov 2016 14:04 Fabien Villepinte, notifications@github.com
wrote:

Hi,

Out of curiosity I tried to reproduce your problem but it works fine with
me (and Humbug mutates the function the same way).

Here my test :

public function test_deep_clone()    {        $o1 = (object) [];        $o1->foo = 'bar';        $o2 = deep_clone($o1);        self::assertEquals('bar', $o2->foo);    }

and the result I got from humbug :

Humbug version 1.0-dev

Humbug running test suite to generate logs and code coverage data...

1 [==========================================================] < 1 sec

Humbug has completed the initial test run successfully.
Tests: 1 Line Coverage: 100.00%

Humbug is analysing source files...

Mutation Testing is commencing on 1 files...
(.: killed, M: escaped, S: uncovered, E: fatal error, T: timed out)

.

1 mutations were generated:
1 mutants were killed
0 mutants were not covered by tests
0 covered mutants were not detected
0 fatal errors were encountered
0 time outs were encountered

Metrics:
Mutation Score Indicator (MSI): 100%
Mutation Code Coverage: 100%
Covered Code MSI: 100%

I could investigate more if you'd share your test.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#194 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABECzRJJjfHqAXXsePEArLq7Jsjdyz5Rks5q6y09gaJpZM4KjsYR
.

@theofidry
Copy link
Member Author

theofidry commented Nov 4, 2016

Hi @villfa, thanks for looking into this :)

Sorry for not actually double checking if that was still the case in a isolated environment, you can however find the real test on nelmio\alice.

To install it:

  • clone + composer install
  • composer bin humbug install
  • vendor-bin/humbug/bin/humbug

The function in question is in src/functions.php.

@villfa
Copy link
Contributor

villfa commented Nov 7, 2016

@theofidry I think the problem is related with your humbug configuration.
The file src/functions.php is excluded so I'm not sure if the function should even be mutated.

@theofidry
Copy link
Member Author

@villfa I excluded it to not have a failure, otherwise it gives the failure I described.

@villfa
Copy link
Contributor

villfa commented Nov 7, 2016

@theofidry Ok, now I get it.
The file src/functions.php is systematically included thanks to the composer autoloader, so when Humbug includes the mutated file the mutated function inside is not evaluated because the condition function_exists('deep_clone') returns true.

@theofidry
Copy link
Member Author

And would cause an error if that was not the case :/

@villfa
Copy link
Contributor

villfa commented Nov 7, 2016

@theofidry I think I found a way to make it work but it's a bit dirty.

In the composer configuration replace src/functions.php by something like src/bootstrap.php.
In the new file src/bootstrap.php:

<?php
if (!getenv('DISABLE_FUNC_LOADING')) {
    require_once __DIR__.'/functions.php';
}

In phpunit.xml.dist define DISABLE_FUNC_LOADING to true.

Finally modify the tests by starting with this (you can improve this part) :

<?php
require_once __DIR__.'/../src/functions.php';
//[...]
class FunctionsTest extends \PHPUnit_Framework_TestCase
//[...]

That way it changes the loading order of the files and the mutated function can be loaded before the original.

I didn't test it with the exact same environment but I think it will work.

@theofidry
Copy link
Member Author

To be honest a simplier way would switch back to an OOP approach even if it's using a static function, but I kinda wanted to avoid that. I'm not fond of doing OOP blindly everywhere especially for something like that.

I think your solution is the only viable one as long as composer doesn't "lazy load" functions :/

@villfa
Copy link
Contributor

villfa commented Nov 8, 2016

This is the occasion to bring some attention to the function autoloading RFC.

@kelunik
Copy link
Contributor

kelunik commented Jan 7, 2017

We're having the same issue with the Amp test suite right now. Humbug should at least be fixed to exclude it automatically then instead of reporting it as escaped mutant.

@theofidry
Copy link
Member Author

@kelunik you mean if Humbug come across a function (not a method), it skips mutants that could be applied to that function? I think it's reasonable, workaround in the meantime other is as said, add it to the ignored files in your config

@kelunik
Copy link
Contributor

kelunik commented Jan 7, 2017

I did that, now it displays all of them as S instead of M.

@kelunik
Copy link
Contributor

kelunik commented Jan 8, 2017

According to @MarkRedeman, only methods should be mutated currently, that's clearly not the case. I added a PR that should fix this and only mutate methods, see #201.

@padraic
Copy link
Collaborator

padraic commented Apr 13, 2017

Thanks for the PR! You're right, only methods should be mutated at present. This is certainly down to focusing on classes, and assuming only classes being tested, though that isn't necessarily where we should end things. I'll raise an issue re functions to at minimum document the limitation.

@theofidry
Copy link
Member Author

Closing as a duplicate of #213

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

5 participants