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

Family bc*-functions mutators (bcmath support) #658

Closed
majkel89 opened this issue Mar 8, 2019 · 7 comments · Fixed by #678
Closed

Family bc*-functions mutators (bcmath support) #658

majkel89 opened this issue Mar 8, 2019 · 7 comments · Fixed by #678

Comments

@majkel89
Copy link
Contributor

majkel89 commented Mar 8, 2019

Similarly to #654 there should be bcmath support

Example

- $c = bcadd($a, b);
+ $c = $a + $b;

I think this mutator should be enabled on demand and not by default.

There should be one mutator for all bcmath functions

@maks-rafalko
Copy link
Member

maks-rafalko commented Mar 9, 2019

A big 👍 for this mutator.

I think this mutator should be enabled on demand and not by default.

Could you please provide arguments?

From my point of view, if developers of a particular project have ext-bcmath installed, and intentionally use slow bc-* functions - they know what are they doing.

And Infection should force them to write a test to prove bc-* function is used with a proper reason.

We probably should skip mutating if extension is not loaded, just for better performance:

// early return
protected function mutatesNode(Node $node): bool 
{
    if (!extension_loaded('bcmath')) {
        return false;
    }

    // ...
}

or even don't add this mutator to mutator list if the extension is not loaded, to prevent its execution for each traversed node.

@sanmai
Copy link
Member

sanmai commented Mar 9, 2019

I think this mutator should be enabled on demand and not by default.

Could you please provide arguments?

For one because bcmath functions work with strings of numbers, and first return value check will have these mutators busted. Example:

var_dump('1' + '2'); \\ int(3)
var_dump(bcadd('1', '2')); \\ string(1) "3"

That assuming strict types are enabled.

@maks-rafalko
Copy link
Member

maks-rafalko commented Mar 9, 2019

We must cast to string then.

- $c = bcadd($a, b);
+ $c = (string) ($a + $b);

@sanmai
Copy link
Member

sanmai commented Mar 9, 2019

This may work, but only before we get into the float territory. Bcmath functions do not exactly work with floats.

var_dump((string) ('11111111111111111111' + '2222222222222222222'));
// string(19) "1.3333333333333E+19"
var_dump(bcadd((string) ('11111111111111111111' + '2222222222222222222'), '1'));
// string(1) "1" - bummer!
var_dump(bcadd(bcadd('11111111111111111111', '2222222222222222222'), '1'));
// string(20) "13333333333333333334"

Otherwise put, just a single test that actually uses very large numbers will catch our mutations. That's a whole lot of mutations for all those functions busted by a single test.

@maks-rafalko
Copy link
Member

maks-rafalko commented Mar 9, 2019

Otherwise put, just a single test that actually uses very large numbers will catch our mutations.

But this is the goal of this mutator - to force developers to write such a test (with big numbers or with floats), isn't it?

What I mean:

We have a method that uses bcadd:

public function add(string $a, string $b): string
{
    return bcadd($a, $b);
}

Then, such test is useless:

assertSame('3', Calculator::add('1', '2'));

and mutant won't be killed. And escaped mutant shows that bcadd function is not needed in this method.

This will force the developer to update such test to:

assertSame('1333333333333333333', Calculator::add('11111111111111111111', '2222222222222222222');

if they really want to work with big numbers, or otherwise replace bcadd($a, $b with $a + $b.

@sanmai
Copy link
Member

sanmai commented Mar 15, 2019

But this is the goal of this mutator - to force developers to write such a test (with big numbers or with floats), isn't it?

Agreed.

I wonder if @majkel89 have some other arguments against having this enabled by default.

@majkel89
Copy link
Contributor Author

majkel89 commented Mar 15, 2019

But this is the goal of this mutator - to force developers to write such a test (with big numbers or with floats), isn't it?

Agreed.

I wonder if @majkel89 have some other arguments against having this enabled by default.

I also agree. Mine only concern was performance but no I think it wont be a gig deal if we disable the mutator when the extension is not loaded.

majkel89 added a commit to majkel89/infection that referenced this issue Mar 25, 2019
majkel89 added a commit to majkel89/infection that referenced this issue Mar 25, 2019
majkel89 added a commit to majkel89/infection that referenced this issue Mar 30, 2019
majkel89 added a commit to majkel89/infection that referenced this issue Mar 30, 2019
majkel89 added a commit to majkel89/infection that referenced this issue Mar 30, 2019
majkel89 added a commit to majkel89/infection that referenced this issue Mar 30, 2019
majkel89 added a commit to majkel89/infection that referenced this issue Mar 30, 2019
majkel89 added a commit to majkel89/infection that referenced this issue Mar 30, 2019
majkel89 added a commit to majkel89/infection that referenced this issue Mar 30, 2019
majkel89 added a commit to majkel89/infection that referenced this issue Mar 30, 2019
majkel89 added a commit to majkel89/infection that referenced this issue Mar 30, 2019
@majkel89 majkel89 mentioned this issue Mar 30, 2019
3 tasks
majkel89 added a commit to majkel89/infection that referenced this issue Apr 1, 2019
maks-rafalko pushed a commit that referenced this issue Apr 20, 2019
* #658 bcmath mutator

* #658 update dev dependencies

* #658 added required tests, refactoring

* cr fixes (#658)

* properly name mappers generators #678
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants