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

feat(checker): add checker api #2240

Merged
merged 6 commits into from
Jun 9, 2020
Merged

Conversation

nicojs
Copy link
Member

@nicojs nicojs commented Jun 1, 2020

Adds Checker plugin type, to be used in #1514 (and can later be used to filter mutants in #1980).

The Checker interface looks like this:

interface Checker {
  init(): Promise<void>;
  check(mutant: Mutant): Promise<CheckResult>;
}
interface CheckResult {
  reason?: string;
  status: CheckStatus;
}
enum CheckStatus {
   Passed = 'passed',
   CompileError = 'compileError',
  // in the future, can include "Ignore" for example.
}

Init

The idea is that, before mutation testing (or even before files are instrumented), we initialize the checker. It can be used as a sanity check. The TypescriptChecker can use this to load all files and compile once (with --no-emit) to make sure errors don't occur under normal circumstances.

Check

Each mutant is checked, one by one. A checker can decide to assign any of the CheckStatus's we've. If we agree we should move stuff used by both the checker api and report api to core.

@simondel @vitaly-rudenko do you agree with this api?

@nicojs nicojs requested a review from simondel June 1, 2020 19:41
@nicojs
Copy link
Member Author

nicojs commented Jun 4, 2020

EDIT: I've included the MutantStatus in the changed files. There you can see I've added 2 new states: Init and Ignored. I've also chosen to implement this with a string enum since that makes it easier to read the states.

We can choose to remove Ignored for now, and add it later so we don't make the mutation switching epic too big.

@nicojs nicojs mentioned this pull request Jun 4, 2020
@Garethp
Copy link
Contributor

Garethp commented Jun 4, 2020

So far looks good. Definitely looks like an interface that I could use to implement a checker for #1980, but I am curious about precedence of Status. If I've got two checkers (TypeChecker, SpecificLinesChecker) for example, would there be two results on the same mutant? Or would any "non-error" (Ignored, Killed, Timedout etc.) response from the first checker simply stop it from being checked by the second checker? In that case, do the checkers run sequentially (despite returning promises)?

Also, when would a checker return a Killed status? My understanding was that checkers were there to try and filter out tests before they reached the test runner stage. What kind of pre-check would you run that would kill a mutant that wouldn't fall into the other status'?

@nicojs
Copy link
Member Author

nicojs commented Jun 5, 2020

I am curious about the precedence of Status. If I've got two checkers (TypeChecker, SpecificLinesChecker) for example, would there be two results on the same mutant?

Great question. The idea I now have is that they would be chained. So the first status != Init wins. So in your use case the checkers should be specified as "checkers": ["SpecificLinesChecker", "TypeChecker"].

Also, when would a checker return a Killed status?

A great observation. The only valid results I can think of right now are

  • Init: proceed to next checker
  • Ignored: filter this mutant, don't run it.
  • CompileError: mutant results in a type error and is therefore invalid

Should we limit the mutant state in the check result to one of these 3 states? We can always add more later if people need it.

@Garethp
Copy link
Contributor

Garethp commented Jun 5, 2020

Yeah, I think limiting it to those three are a good idea, it makes it a lot more clear when implementing as an outsider to only have those options

@Garethp
Copy link
Contributor

Garethp commented Jun 6, 2020

Oh, I noticed that initialize doesn't accept any arguments. How would the plugin accept external input? Does it need some definitions for dependency injecting settings/arguments?

@nicojs
Copy link
Member Author

nicojs commented Jun 6, 2020

Yes, good observation once more :). It will be able to let a bunch of stuff be injected. Like all plugins.

See https://www.github.com/stryker-mutator/stryker-handbook/tree/master/stryker%2Fapi%2Fplugins.md

@nicojs
Copy link
Member Author

nicojs commented Jun 6, 2020

I've renamed the MutantStatus enum to CheckStatus. It can now only have 3 results:

enum CheckStatus {
  Ok = 'ok',
  Ignore = 'ignore',
  CompileError = 'compileError',
}

We'll probably remove ignored before merging this, in order to keep the scope of mutation switching limited. We'll add the ignored state in Stryker soon after mutation switching is implemented.

@Garethp what do you think of the Ok status. Maybe we can think of a better word?

@simondel
Copy link
Member

simondel commented Jun 6, 2020

Perhaps Passed

@nicojs
Copy link
Member Author

nicojs commented Jun 6, 2020

Yes. That might be better. One passes a check.

@Garethp
Copy link
Contributor

Garethp commented Jun 6, 2020

Passed sounds good. This interface looks good. I can implement a SpecificLinesFilter checker once this API is hooked in so that you can get feedback on that specific use-case before release

Removed Ignored status for now
@nicojs
Copy link
Member Author

nicojs commented Jun 6, 2020

This interface looks good. I can implement a SpecificLinesFilter checker once this API is hooked in so that you can get feedback on that specific use-case before release

Sure you can! We can also add the specific line number and column number location to the mutant. Right now it only contains range, which you can use to calculated it to lines/columns, but that is error prone.

Move the `Mutant` interface from from `api/mutant` to `api/core`.
It is now also used in the new checker api.
@bartekleon
Copy link
Member

The reason is ?string? I don't like this version that much :/ I think there should always be reason

@nicojs
Copy link
Member Author

nicojs commented Jun 6, 2020

I've renamed Ok to Passed. I've also removed Ignored for now. I've also moved Mutant from api/mutant to api/core (since it is now used in 2 api packages).

Ready for review @simondel

@nicojs nicojs marked this pull request as ready for review June 6, 2020 19:21
@vitaly-rudenko
Copy link

I haven't investigated the stryker's source code yet, but this feature looks very good. Filtering mutants would solve my problem for sure.
Implementing a custom checker will be easy with such API.

Thank you for your hard work!

Copy link

@vitaly-rudenko vitaly-rudenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it! Simple yet powerful feature.

@nicojs nicojs merged commit d463f86 into epic/mutation-switching Jun 9, 2020
@nicojs nicojs deleted the feat/checker-api branch June 9, 2020 20:34
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

Successfully merging this pull request may close these issues.

5 participants