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

Add mutation for conditional (ternary) operator #2581

Open
causa-prima opened this issue Jun 21, 2023 · 14 comments · May be fixed by #2583
Open

Add mutation for conditional (ternary) operator #2581

causa-prima opened this issue Jun 21, 2023 · 14 comments · May be fixed by #2583
Labels
Area: Mutator new mutators or change to existing one 🚀 Feature request New feature or request

Comments

@causa-prima
Copy link

Is your feature request related to a problem? Please describe.
I have some code that prefixes a string conditionally:

val = val.Contains(prefix)
       ? val
       : $"{prefix}{val}";

I have a test that checks that some strings without that prefix get prefixed, but I do not have any test that checks whether already prefixed strings also get that prefix added. Stryker didn't find that issue as I expected it to.

Describe the solution you'd like
I propose adding two mutations for the conditional (ternary) operator:
Original: a ? b : c
Mutation 1: b
Mutation 2: c

Describe alternatives you've considered
I can't think of any.

Additional context
None.

@causa-prima causa-prima added the 🚀 Feature request New feature or request label Jun 21, 2023
@rouke-broersma rouke-broersma added the Area: Mutator new mutators or change to existing one label Jun 21, 2023
@michalusio
Copy link

This mutator does not seem like it would be hard to add, so if nobody is objecting, I will try to add it.

@dupdob
Copy link
Member

dupdob commented Jun 21, 2023

ternary expression have a specific type resolution logic when b and c are of different types. Not sure what will happen when cascading ternary operators, but one can assume type resolution logic will not be able to resolve it. So, this will be the cause for compilation errors, but they should be rolled back.

I guess this mutator would benefit for semantic compilation, when/if it is activated.

@michalusio
Copy link

@dupdob That is true, replacing the ternary with one of the branches could affect type resolution.
I was thinking of just replacing the condition into true and false.
The only problem I see with that is un-killable mutants if the condition was always true/always false, but that's probably on the developer for writing that kind of expression - Stryker will still show something's not right there, so I think it's a gain.
What do you think?

@michalusio michalusio linked a pull request Jun 21, 2023 that will close this issue
@dupdob
Copy link
Member

dupdob commented Jun 22, 2023

un killable mutants should not exist here, they would be the sign that the ternary operator is not needed.
Coming back to the feature request: I was pretty convinced there was already a mutator for ternary operator, but failed to find it at first.
Then I ran into NegateConditionMutator: it is suppose to mutate conditions for if, while and conditional expressions (a.k.a ternary operator).
Could you please dig into why it did not work for you instead of adding a new mutator ?

@dupdob
Copy link
Member

dupdob commented Jun 22, 2023

you can have a look at this unit test:
https://github.com/stryker-mutator/stryker-net/blob/32b0116f0c711c0f95b0d55fa84a1e35b38f475a/src/Stryker.Core/Stryker.Core.UnitTest/Mutators/NegateConditionMutatorTests.cs#L48C5-L63

granted, this is a different mutations, but it looks to me like it should be give satisfactory results

@michalusio
Copy link

@dupdob The mutator you have linked mutates conditional expressions by creating a negation of the expression.
The mutation suggested in this issue pertains to mutating the whole condition to true and false mutations.
That being said, I could modify the NegateConditionMutator instead of making a new. I would have to change it's name, though.

@dupdob
Copy link
Member

dupdob commented Jun 22, 2023

I think both mutations are equivalent: each survives if the developer failed to have tests for both branch of the expression.
That's why I suggested this. But if you want to add a "true/false" mutation, you need to add a specific mutator for this.

Have you looked into why the negate condition mutator failed to identify this problem in your project?

@causa-prima
Copy link
Author

My explanation was that the mutations Stryker performs simply do not mutate the code in the way proposed, and all the mutations it does are not sufficient to find the missing test.

As the actual code is a bit different from the example I showed in my first post, I've created a simple example project that you can check for yourself:
MutatorExample.zip

The method I'm testing:

namespace MutatorExample
{
    public static class Class1
    {
        public static string AddImagePrefix(string logo)
        {
            logo = logo.Contains("data:image/jpg;base64,")
                ? logo
                : $"data:image/jpg;base64,{logo}";

            return logo;
        }
    }
}

The test for it:

namespace MutatorExampleTests
{
    using FluentAssertions;
    using MutatorExample;

    public class Tests
    {
        [SetUp]
        public void Setup()
        {
        }

        [Test]
        [TestCase("a")]
        [TestCase("any")]
        [TestCase("any base64 coded string")]
        [TestCase("any base64 coded string, that is a bit longer then that before")]
        public void AddImageprefix_ReturnsBase64String(string originalString)
        {
            // Arrange

            // Act
            var result = Class1.AddImagePrefix(originalString);

            // Assert
            result.Should().BeEquivalentTo($"data:image/jpg;base64,{originalString}");
        }
    }
}

Stryker only does mutate the strings or negates the condition, but that does not fetch the missing test.
grafik
grafik
grafik

@dupdob
Copy link
Member

dupdob commented Jun 23, 2023

thanks for the explanation. Yes indeed! There are 3 possible situations here:

  1. conditional expression not covered ==> Stryker flags it
  2. one branch of the conditional expression covered ==> Negate condition mutator mutants will be killed
  3. both branches covered ==> Mutants are killed

You are right, it seems a dedicated mutator is revelant. I suggest then that you remove support for conditional expressions from the Negate condition mutator, as those mutants would be less interesting than the new ones.

@michalusio
Copy link

@dupdob
So if I remove the conditional expression from the NegateCondition mutator, it should be fine then?

@dupdob
Copy link
Member

dupdob commented Jun 25, 2023

yes, this will avoid having duplicate/similar mutants

@psfinaki
Copy link
Contributor

Cool stuff!

It took me a while to understand the missing piece here so here is the mutated code that Stryker wouldn't flag:

  private const string crap = "crap";

  public static string AddImagePrefix(string logo)
  {
      logo = logo.Contains("data:image/jpg;base64,")
          ? crap
          : $"data:image/jpg;base64,{logo}";

      return logo;
  }

Missing tests would be

[Test]
[TestCase("data:image/jpg;base64,")]
[TestCase("blah,data:image/jpg;base64,blah")]
public void AddImageprefix_KeepsInputStringWhenNeeded(string originalString)
{
    // Arrange

    // Act
    var result = AddImagePrefix(originalString);

    // Assert
    result.Should().BeEquivalentTo($"data:image/jpg;base64");
}

@causa-prima
Copy link
Author

As I do not see any changes here, I suspect that nothing was changed yet, even though @michalusio proposed to try to add this if there are no objections. Is something missing, or did none get around to do this yet?

@Liam-Rougoor
Copy link
Contributor

Hi @causa-prima !
I'm taking a look at the PR again, but it seems there are still some unresolved comments and questions. I'll check if they are still relevant and try and merge soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Mutator new mutators or change to existing one 🚀 Feature request New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants