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

Converting Stryker4S into Stryker4JVM supporting Scala and Kotlin #1291

Draft
wants to merge 438 commits into
base: master
Choose a base branch
from

Conversation

mmulder135
Copy link

Stryker4S provides mutation testing for Scala, while Stryker4K does this for Kotlin. Since these projects are developed in parallel, and they are both JVM languages, they have large parts of code with exactly the same functionality (such as file IO, logging and reporting). The main problem to be solved is thus the code duplication between the different code bases. In addition to this, we should try to ensure that the shared interface can also be implemented by other JVM languages at a later time.

s2176416 and others added 30 commits December 13, 2022 23:42
Updated dependencies between the files
Updated some imports and dependencies
Added IOReporter and wrapper for default core reporters
Fixed many bad references to code from scala-mutator
Fixed config encoding/decoding for correct stryker4jvm class (implicits hell makes it really hard to determine where the issue is coming from)
Converted the reporters extending from Java 'Reporter' to extending 'IOReporter', there now exists a wrapper to convert pure java reporters to IOReporters
Fixed numerous import issues and type issues
Created java classes for some of the scala mutationtesting.elements classes
Converted stryker4jvm and stryker4jvm-mutator-scala to use new java core classes
Added many converters between core and mutationtesting.elements scala types in Stryker4jvmCoreConversions. Functions declared 'asCoreElement' and 'asMutationElement'.
Fixed an issue in splitIgnoredAndFound which was returning an empty stream when size of found was non-zero.
Removed intransitive() dependency keyword from build.sbt
sbt stryker4jvm/compile compiles with no errors
…ble to compile

Stryker4jvm's tests cannot compile, but they never had
Commented 'createFinishedRunEvent' in TestData.scala of mutator-scala as FinishedRunEvent is removed from core
* Added a separate location for supported language, implementing the kotlin mutator

* obligatory extra scalafmt commit

* Optimize imports to remove warning

Co-authored-by: Michael Mulder <[email protected]>
# Conflicts:
#	.github/workflows/ci.yml
#	core/src/main/scala/stryker4s/Stryker4s.scala
#	core/src/main/scala/stryker4s/extension/exception/stryker4sException.scala
#	core/src/main/scala/stryker4s/mutants/findmutants/MutantMatcher.scala
#	core/src/main/scala/stryker4s/mutants/tree/MutantInstrumenter.scala
#	core/src/main/scala/stryker4s/run/MutantRunner.scala
#	core/src/main/scala/stryker4s/run/RollbackHandler.scala
#	core/src/test/scala/stryker4s/extension/exception/Stryker4sExceptionTest.scala
#	core/src/test/scala/stryker4s/mutants/AddAllMutationsTest.scala
#	core/src/test/scala/stryker4s/mutants/RollbackTest.scala
#	core/src/test/scala/stryker4s/mutants/findmutants/MutantMatcherTest.scala
#	core/src/test/scala/stryker4s/mutants/tree/MutantInstrumenterTest.scala
#	core/src/test/scala/stryker4s/report/mapper/MutantRunResultMapperTest.scala
#	core/src/test/scala/stryker4s/run/MutantRunnerTest.scala
#	core/src/test/scala/stryker4s/testutil/stubs/TestRunnerStub.scala
#	stryker4jvm-mutator-scala/src/main/scala/stryker4jvm/mutator/scala/mutants/findmutants/MutantMatcher.scala
Removed redundant scala files
Copy link
Member

@hugo-vrijswijk hugo-vrijswijk left a comment

Choose a reason for hiding this comment

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

Some remarks about the overall layout and some remarks from an initial glance. I haven't really looked into the code yet, I'll probably be able to do that in the coming days.

I've also put out a poll to get some community feedback on the naming:

@@ -1,19 +1,7 @@
<!---
Copy link
Member

Choose a reason for hiding this comment

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

Why was this removed?

Choose a reason for hiding this comment

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

No idea, will be restored

strategy:
fail-fast: false
matrix:
java-version: [11, 17]
os: ['ubuntu-latest', 'windows-latest']
java-version: [ 11, 15 ]
Copy link
Member

Choose a reason for hiding this comment

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

Stryker4s supports the current LTS version of Java, so we should keep testing on it. 15 is EOL so we don't have to test on it

run: sbt 'compile; ++2.13 test'
- name: Build core with Maven
working-directory: ./stryker4jvm-core
run: mvn clean install
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
run: mvn clean install
run: mvn install

You _rarely need to do a clean install for maven. See here why. Adding -B tells maven you're running in 'batch' mode (for cleaner CI output)

Looks like it is needed here, but you don't need the clean

Choose a reason for hiding this comment

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

When reinstalling locally it is necessary to clean sometimes which is probably why it also exists in the CI but indeed should not have to clean here.

# Sbt version that is tested (1.1.1) doesn't work on Java 17
java-version: 11
distribution: 'corretto'
java-version: 15
Copy link
Member

Choose a reason for hiding this comment

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

Same here for the Java version. Ideally I'd like to see as little changes as possible on the CI script or integration tests in this PR

README.md Outdated

```scala
addSbtPlugin("io.stryker-mutator" % "sbt-stryker4s" % stryker4sVersion)
addSbtPlugin("io.stryker-mutator" % "stryker4jvm-plugin-sbt" % stryker4jvmVersion)
Copy link
Member

Choose a reason for hiding this comment

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

The standard for sbt plugins is to follow sbt-$name, so in this case it would be sbt-stryker4jvm.

It might also be useful to still publish the old artifact (possibly with a scala-steward migration

FileResolver has been ignored as it relies on fs2, this may instead be added to stryker4jvm

Reporting:
Converted scala Reporter to abstract java class without any IO (wrappers may be necessary in stryker4jvm if we wish to work with IO there);
Copy link
Member

Choose a reason for hiding this comment

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

Could the reporters not also be inside stryker4jvm? They shouldn't be used by individual language mutators

Choose a reason for hiding this comment

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

The merge-notes are outdated and are merely used during development of stryker4jvm to clarify where some things have been moved. They should not have ended up in the PR and will be removed.


class SbtLogger(sbtLogger: SbtInternalLogger) extends Logger {

def log(level: Level, msg: => String): Unit = sbtLogger.log(toSbtLevel(level), msg)
Copy link
Member

Choose a reason for hiding this comment

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

I don't see this class moved anywhere. How does logging now work inside the plugins?

Choose a reason for hiding this comment

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

SbtLogger is moved to module stryker4jvm-plugin-sbt. The SbtLogger class is private but a new FansiSbtLogger class has been created.

libraryDependencies += "com.thesamet.scalapb" %% "compilerplugin" % "0.11.11"

// Coverage
addSbtPlugin("org.scoverage" % "sbt-scoverage" % "2.0.6")
Copy link
Member

Choose a reason for hiding this comment

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

I don't think adding code coverage should really be a goal of this PR. It should probably be separate (or better, use mutation coverage)

Choose a reason for hiding this comment

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

I think it exists because of the test coverage QA we set during the project, up to you if you want to keep it in

import fansi.Str
import stryker4jvm.core.logging.{LogLevel, Logger}

class FansiLogger(val coreLogger: Logger) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a clean solution for the loggers could be to have the same Logger class in core with just Strings in core, but to have this class (maybe renamed to ColorLogger) extend the base Logger and add the Fansi methods and color checking logic. Then the language mutators could have a Logger interface passed, while all the projects 'below' that would get a ColorLogger injected, which would be the same logger instantation, but just with a 'wider' interface

Choose a reason for hiding this comment

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

Because fansi.str automatically converts to strings it was unclear at times whether someone was actually using a 'FansiLogger' or a 'core Logger', for this reason it was decided to split it but the proposed solution should work as well.

This is the contribution guide for Stryker4s. Great to have you here! Here are a few ways you can help to make this project better.

## Developing on stryker4jvm:
Currently, stryker4jvm is using 3 build tools for all its modules. Stryker4jvm-core is managed by maven,
Copy link
Member

Choose a reason for hiding this comment

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

I think it would also help to have a picture (or ascii diagram) of the project layout as it's quite a bit more complex now

Choose a reason for hiding this comment

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

I agree, will add one

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