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

How do I simply exclude method from class #1360

Closed
see-quick opened this issue Nov 3, 2024 · 9 comments
Closed

How do I simply exclude method from class #1360

see-quick opened this issue Nov 3, 2024 · 9 comments

Comments

@see-quick
Copy link
Contributor

Hello, how can one configure within the JUnit5 plugin exclusion of the specific method? Imagine simple example:

package io;

import org.junit.jupiter.api.Tag;

import java.util.List;
import java.util.stream.Collectors;

public class MyClass {

    /**
     * Filters a list of strings, returning only those that start with a specified prefix.
     *
     * @param input  the list of strings to filter
     * @param prefix the prefix to filter by
     * @return a list of strings that start with the given prefix
     */
    public List<String> filterByPrefix(List<String> input, String prefix) {
        return input.stream()
            .filter(s -> s.startsWith(prefix))
            .collect(Collectors.toList());
    }
}

and dummy tests, which does nothing (expected) because we want to exclude such method.

package io;

import org.junit.jupiter.api.Test;


public class MyClassTest {

    @Test
    public void testFilterByPrefix() {
        // Dummy nothing
    }
}

Now when I run Pitest with the default configuration I get
image

which is fine because I didn't exclude any method yet. But now when I add to the plugin configuration

 <excludedMethods>
        <param>io.MyClass::lambda$filterByPrefix$*</param>
</excludedMethods>

nothing really changed. I tried various stuff:

  1. around without / with `/
  2. escaped $
  3. etc.

My question is that possible to simply exclude a method? Because this is really great use case when you have a class where logic is basically mainly tasted on the Integration level and not the Unit level. I tried the same with the this works well without any problems. Thanks for the quick answer.

@hcoles
Copy link
Owner

hcoles commented Nov 4, 2024

The excludedMethods parameter is designed to exclude all methods matching a given glob. It does not match against the class name containing it, so "io.MyClass::" guarantees nothing will ever match.

To exclude specific methods you have three options.

  1. Annotate them with an annotation named Generated, CoverageIgnore, or DoNotMutate with at least class level retention
  2. Create a custom mutation filter
  3. Use the arcmutate base exclusion extensions

@see-quick
Copy link
Contributor Author

see-quick commented Nov 4, 2024

The excludedMethods parameter is designed to exclude all methods matching a given glob. It does not match against the class name containing it, so "io.MyClass::" guarantees nothing will ever match.

To exclude specific methods you have three options.

  1. Annotate them with an annotation named Generated, CoverageIgnore, or DoNotMutate with at least class level retention
  2. Create a custom mutation filter
  3. Use the arcmutate base exclusion extensions
  1. Annotate them with an annotation named Generated, CoverageIgnore, or DoNotMutate with at least class level retention

That annotation lives somewhere in the public API of pitest? Or should the user define his annotation (i.e., DoNotMutate, CoverageIgnore or Generated) and use it inside the code?

@hcoles
Copy link
Owner

hcoles commented Nov 4, 2024

Any annotation with correct name (and retention) will work.

You can use

https://mvnrepository.com/artifact/com.arcmutate/pitest-annotations/1.3.3

If you don't wish to define your own.

@see-quick
Copy link
Contributor Author

see-quick commented Nov 4, 2024

Hmm I can see that for simple methods is excluded easily f.e.,

@Override
    @DoNotMutate
    public void stop() {
        if (this.isZooKeeperBasedKafkaCluster()) {
            // firstly we shut-down zookeeper -> reason: 'On the command line if I kill ZK first it sometimes prevents a broker from shutting down quickly.'
            this.zookeeper.stop();
        }

        // stop all kafka containers in parallel
        this.brokers.stream()
            .parallel()
            .forEach(KafkaContainer::stop);
    }

but for a method with another local lambda method, it does not ignore it f.e.

@SuppressWarnings({"CyclomaticComplexity", "NPathComplexity"})
    @Override
    @DoNotMutate
    public void start() {
        Stream<KafkaContainer> startables = this.brokers.stream();
        try {
            Startables.deepStart(startables).get(60, TimeUnit.SECONDS);
        } catch (InterruptedException e) {
            Thread.currentThread().interrupt();
            throw new RuntimeException("Interrupted while starting Kafka containers", e);
        } catch (ExecutionException | UnsupportedKraftKafkaVersionException e) {
            Throwable cause = e.getCause();
            if (cause instanceof UnsupportedKraftKafkaVersionException) {
                throw (UnsupportedKraftKafkaVersionException) cause;
            } else {
                throw new RuntimeException("Failed to start Kafka containers", e);
            }
        } catch (TimeoutException e) {
            throw new RuntimeException("Timed out while starting Kafka containers", e);
        }

        if (this.isZooKeeperBasedKafkaCluster()) {
            Utils.waitFor("Kafka brokers nodes to be connected to the ZooKeeper", Duration.ofSeconds(1).toMillis(), Duration.ofMinutes(1).toMillis(),
                () -> {
                    Container.ExecResult result;
                    try {
                        result = this.zookeeper.execInContainer(
                            "sh", "-c",
                            "bin/zookeeper-shell.sh zookeeper:" + StrimziZookeeperContainer.ZOOKEEPER_PORT + " ls /brokers/ids | tail -n 1"
                        );
                        String brokers = result.getStdout();

                        LOGGER.info("Running Kafka brokers: {}", result.getStdout());

                        return brokers != null && brokers.split(",").length == this.brokersNum;
                    } catch (IOException | InterruptedException e) {
                        Thread.currentThread().interrupt();
                        throw new RuntimeException("Failed to execute command in ZooKeeper container", e);
                    }
                });
        } else if (this.isKraftKafkaCluster()) {
            // Readiness check for KRaft mode
            Utils.waitFor("Kafka brokers to form a quorum", Duration.ofSeconds(1).toMillis(), Duration.ofMinutes(1).toMillis(),
                () -> {
                    try {
                        for (KafkaContainer kafkaContainer : this.brokers) {
                            Container.ExecResult result = ((StrimziKafkaContainer) kafkaContainer).execInContainer(
                                "bash", "-c",
                                "bin/kafka-metadata-quorum.sh --bootstrap-server localhost:" + StrimziKafkaContainer.INTER_BROKER_LISTENER_PORT + " describe --status"
                            );
                            String output = result.getStdout();

                            LOGGER.info("Metadata quorum status from broker {}: {}", ((StrimziKafkaContainer) kafkaContainer).getBrokerId(), output);

                            if (output == null || output.isEmpty()) {
                                return false;
                            }

                            // Check if LeaderId is present and valid
                            final Pattern leaderIdPattern = Pattern.compile("LeaderId:\\s+(\\d+)");
                            final Matcher leaderIdMatcher = leaderIdPattern.matcher(output);

                            if (!leaderIdMatcher.find()) {
                                return false; // LeaderId not found
                            }

                            String leaderIdStr = leaderIdMatcher.group(1);
                            try {
                                int leaderId = Integer.parseInt(leaderIdStr);
                                if (leaderId < 0) {
                                    return false; // Invalid LeaderId
                                }
                            } catch (NumberFormatException e) {
                                return false; // LeaderId is not a valid integer
                            }

                            // If LeaderId is present and valid, we assume the broker is ready
                        }
                        return true; // All brokers have a valid LeaderId
                    } catch (IOException | InterruptedException e) {
                        Thread.currentThread().interrupt();
                        throw new RuntimeException("Failed to execute command in Kafka container", e);
                    }
                });
        }
}

and I can see that such lambda method has mutants spawned

image

@hcoles
Copy link
Owner

hcoles commented Nov 5, 2024

Yes, that looks to be an issue. I'll see if I can take a look for the next release.

@see-quick
Copy link
Contributor Author

Yes, that looks to be an issue. I'll see if I can take a look at the next release.

Hmmm, maybe you can point me to what should be changed (of some radius where to find such a problem) and I can help.

@hcoles
Copy link
Owner

hcoles commented Nov 6, 2024

Contributions are always welcome.

The logic here

final List<Predicate<MutationDetails>> methods = clazz.methods().stream()

Needs to be expanded so that, instead of just scanning for directly annotated methods, the code compiles a list of annotated methods in one scan, then extracts a list of lambdas etc based on those names in a 2nd scan.

Complications to consider include

  • Overloaded methods (i.e where two methods exist with the same name but different signatures and only 1 is annotated)
  • Lambdas within lambdas within lambdas

@see-quick
Copy link
Contributor Author

After implementing [1] I tested with the source I posted above. Such an issue is resolved. Let me know what you think about that and if I forgot something...

[1] - #1362

@see-quick
Copy link
Contributor Author

Closing this issue as #1362 is completed.

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

No branches or pull requests

2 participants