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

[WIP] Add CanonicalDataParser (issue #331) #347

Merged
merged 1 commit into from
Mar 31, 2017

Conversation

abo64
Copy link
Contributor

@abo64 abo64 commented Mar 27, 2017

This is a first shot to get started. There are a couple of issues for discussion here:

  • CanonicalDataParser parses the new json format: I used scala-parser-combinators because it is basic enough for me (it always creates simple Maps) to no not invest too much work. But feel free to switch to some more sophisticated framework like play-json.
  • The additional nesting of test cases in LabeledTestGroup is just flattened out, that is not reflected in the resulting test suite code. So far there are too few and too insignificant instances of this to elaborate on this. That may change in the future.
  • I used twirl as a template engine in order to remove some of the magic of code generation and to be more flexible should there be more complicated or different test suites to be generated. In this case one could just use some other template.
  • The API is TestSuiteBuilder. CanonicalDataParser is not intended to be called directly.
  • I tried to add a few TestGenerators to get an impression how this could look like in practice, but it could be refined in some ways. We could modify them to generate the .scala, say.
  • Notably hello-world has changed considerably, so I changed the respective code already.
  • We could think of introducing a package testgen. This has the usual advantages to clean up the code base, for example using a package object for type declarations and such. But I did not do it for now.

And there could be more things to debate. Please let me know what you think.

Copy link
Member

@ErikSchierboom ErikSchierboom 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 the approach. The generators look nice and clean and will be easy to add. Flattening the exercises is also currently what I do in the C# track.

@ricemery
Copy link
Member

This looks really good. But, a couple of things.

  1. projects/plugins.sbt seems to be missing. I couldn't load the sbt project without creating plugins.sbt with the twirl info.

  2. Is there a trick to build/run? I am unfamiliar with twirl. So, I am probably missing something. I get the following errors when I try to compile from sbt.

[error] .../testgen/src/main/twirl/funSuiteTemplate.scala.txt:1: not found: type TestSuiteData [error] @(data: TestSuiteData)@for(imp <- data.imports) {import @imp} [error] ^ [error] .../testgen/src/main/twirl/funSuiteTemplate.scala.txt:1: not found: type TestSuiteData [error] @(data: TestSuiteData)@for(imp <- data.imports) {import @imp} [error] ^ [error] .../testgen/src/main/twirl/funSuiteTemplate.scala.txt:3: not found: value TestSuiteBuilder [error] @import TestSuiteBuilder._ [error] ^ [error] .../testgen/src/main/twirl/funSuiteTemplate.scala.txt:12: not found: type TestSuiteData [error] }} [error] ^ [error] four errors found [error] (compile:compileIncremental) Compilation failed [error] Total time: 1 s, completed Mar 27, 2017 8:52:29 PM

I have tried running sbt twirlCompileTemplates then compile. sbt twirlCompileTemplates did not solve the errors.

I am going to be unreachable until the end of the week. So, if you guys want to merge this, I have no problem with that. I would just like to get this running on my box so I could experiment.

Thanks @abo64 !

@abo64 abo64 force-pushed the issue-331-test-suite-generator branch 2 times, most recently from d4c5018 to 9503d76 Compare March 28, 2017 06:02
@abo64
Copy link
Contributor Author

abo64 commented Mar 28, 2017

Sorry, I had tried it in a slightly different project and apparently did not copy over all required files to this branch. The changes I had to make:

  • twirl templates seem to need a proper package for imports to work. So I added package testgen.
  • It also needs needs a higher sbt-version 0.13.13, I added that in project/build.properties.
    But we should think of upgrading sbt in general?!
  • I also added the missing project/plugins.sbt.

Now that there is this package testgen one could think about having a package object to stow away some type declarations and case classes. Haven't got the time for this, maybe later.

Hope it works for you now, @ricemery ?
I think we should only merge when this works properly and everybody feels ok about it. No need to hurry.

@ErikSchierboom
Copy link
Member

Well, as they say, "the proof of the pudding is in the eating". I see you have already added some small, rather simple test generators, but maybe adding a somewhat more complex would give even more insight?

@ricemery
Copy link
Member

I am able to build now. I updated the PangramsTestGenerator as well. No time to muck with Pull request before my flight:)
I didn't handle the case where quotes needed to be escaped in the isPangram argument. Otherwise it worked well.

import testgen._
import TestSuiteBuilder._
import java.io.File

object PangramsTestGenerator {
  def main(args: Array[String]): Unit = {
    val file = new File("src/main/resources/pangram.json")

    def fromLabeledTest(argNames: String*): ToTestCaseData =
      withLabeledTest { sut => labeledTest =>
        val args = sutArgs(labeledTest.result, argNames: _*)
        val isPangram = labeledTest.property.mkString
        val sutCall =
          s"""Pangrams.$isPangram($args)"""
        val expected =
          labeledTest.expected.fold(Function.const("true"), x => s"$x")

        TestCaseData(labeledTest.description, sutCall, expected)
      }


    val code = TestSuiteBuilder.build(file, fromLabeledTest("input"))
    println(s"-------------")
    println(code)
    println(s"-------------")
  }
}

@abo64 abo64 force-pushed the issue-331-test-suite-generator branch 3 times, most recently from 189d766 to 821f902 Compare March 29, 2017 05:45
@abo64
Copy link
Contributor Author

abo64 commented Mar 29, 2017

@ErikSchierboom suggested to try beer-song and clock. The latter is really tough, and I left it alone for now (maybe it is too unique and complicated to bother about?). To handle the former I added two things:

  • Strings are turned into raw Strings when they contain a double quote or newline character.
    This might even handle the respective pangram cases, @ricemery ?
    It works ok for beer-song but might look a bit strange or ugly in other cases, though?
  • I added function sutArgsAlt to handle multiple functions under test and their arguments.
    Actually, I find it quite readable and we could consider to replace sutArgs by it?

I changed PangramsTestGenerator with your code, @ricemery .
Remark: One side effect of TestSuiteBuilder could be that we enforce stringent naming conventions using the function names suggested in x-common, but also the SUT. In this case: The exercise is called "pangram", so the SUT should also be named "Pangram" (not "Pangrams"). For that purpose TestSuiteBuilder contains functions testSuiteName and sutName. Or is it too constraining?

I am not overly happy with this currying trick of fromLabeledTest/withLabeledTest. Not very readable and convenient to use. Perhaps this can be replaced by some function parameters with default values. I will see how this works out.

EDIT: added FoodChainGenerator, too.

@abo64 abo64 force-pushed the issue-331-test-suite-generator branch from 821f902 to e7ba3aa Compare March 29, 2017 07:02
@ricemery ricemery merged commit 4080052 into exercism:master Mar 31, 2017
@abo64 abo64 deleted the issue-331-test-suite-generator branch July 4, 2017 04:25
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.

3 participants