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

Make process runner configurable #38

Merged
merged 10 commits into from Aug 31, 2018
Merged

Conversation

hugo-vrijswijk
Copy link
Member

@hugo-vrijswijk hugo-vrijswijk commented Aug 13, 2018

Fixes #36

Added the option to configure the test command that the ProcessRunner runs. It can be configured with a command-runner test-runner. Future test-runners could be configured in a way similar to Stryker's test-runners and read in the TestReader.

I added a new TestRunner config value instead of adding the correct MutantRunner as it made more sense to me to keep configuration in the Config object instead of adding an implementation to it. I am not sure about the naming of the classes yet, I think they are confusing. But also don't know what would be better.

@ghost ghost assigned hugo-vrijswijk Aug 13, 2018
@ghost ghost added the in progress label Aug 13, 2018
@hugo-vrijswijk hugo-vrijswijk added the enhancement New feature or request label Aug 14, 2018
@legopiraat
Copy link
Contributor

Somehow I feel icky about the command because if the user defines the full command we will not be able to preserve the session for example sbt which we would like to do. I think I would be more of a fan to split of the process runner command from the other command, something like:

{
    command-runner: {
          runner: "sbt"
          command: "test -DskipStuff=true"
     }
 }

For sure this will make it usable for all build tools but it has some big downsides.

@nicojs
Copy link
Member

nicojs commented Aug 16, 2018

Great that you're using the same name here: command-runner is also the name Stryker uses

@hugo-vrijswijk
Copy link
Member Author

@legopiraat I imagine with a SBT (or other) plugin the syntax would be like this, as it won't be a command-runner:

test-runner: "sbt"

I'm not opposed to splitting the command and args into separate values. That would also make it easier to put out a warning when we have a plugin to use instead.

@legopiraat
Copy link
Contributor

If you don't mind I think it would be a good idea to split it up. And indeed it will be easier for us to putt out the notifications I suppose a lot of users would appreciate that.

@hugo-vrijswijk
Copy link
Member Author

I've split the command and args into separate values

/** Reads a test-runner from the ConfigReader
* For now, this is always a command-runner object, but in the future can be expanded to include other test-runners
*/
object TestRunnerReader extends ConfigReader[TestRunner] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the custom ConfigReader, maintainability is not that good. To get better maintainability we could and should make it an ADT and use option one.

https://pureconfig.github.io/docs/complex-types.html

trait TestRunner

case class CommandRunner(command: Command) extends TestRunner {
override def toString: String = s"""{
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make an issue for these toString methods, it's already getting tedious in my opinion.

We should be able to generate this format really easy I guess.

new LogRunReporter()
)

stryker4s.run()

def resolveRunner()(implicit config: Config): MutantRunner = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be private

@hugo-vrijswijk
Copy link
Member Author

Turns out Pureconfig also has a ConfigWriter function, for writing objects to Typesafe config. I've implemented this, which also fixes #39. I've also removed the TestRunnerReader completely, though the configuration is slightly different. Configuring the TestRunner would now look like this:

test-runner: {
  type=commandrunner
  command: "mvn"
  args: "clean test" 
}

I'm not completely opposed to the type argument, as I think it's still clear what the option does.

@legopiraat legopiraat merged commit cc4c5d0 into master Aug 31, 2018
@ghost ghost removed the in progress label Aug 31, 2018
@legopiraat legopiraat deleted the configurable-process-runner branch August 31, 2018 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants