JSON validation rendered by the RamlTypeGenerator should take into consideration
the additional constraints specified in RAML (e.g. minItems, max, pattern).
This will reduce boilerplate in higher-level validation code and consistently
enforce constraints declared in RAML at JSON-parsing time.
Details
- Reviewers
aquamatthias timcharper • jasongilanfarr - Commits
- rMARATHON031ee2b36d24: fix RAML spec to get integration tests passing
rMARATHONa3e71e830415: more readable constraint code
rMARATHON06f157b52734: validation constraints for numeric, string, and array types
rMARATHONa77dcb89ad6c: validation constraints for numeric, string, and array types
rMARATHONcc14213ac49b: Merge 03bf79fec981f4716e54b225459ef3ac516d6730 into…
rMARATHON66a6f7794ff6: validation constraints for numeric, string, and array types
sbt test
sample of generated code:
object Network { import play.api.libs.json.Reads._ import play.api.libs.functional.syntax._ implicit object playJsonFormat extends play.api.libs.json.Format[Network] { def reads(json: play.api.libs.json.JsValue): play.api.libs.json.JsResult[Network] = { val name = json.\("name").validateOpt[String](play.api.libs.json.JsPath.read[String](maxLength[String](63) keepAnd minLength[String](1) keepAnd pattern("^[a-z0-9]([-a-z0-9]*[a-z0-9])?$".r))) ...
demo:
scala> play.api.libs.json.Json.parse("""{ "name": "this is not a valid name. this is not a valid name. this is not a valid name. this is not a valid name. this is not a valid name." }""").as[mesosphere.marathon.raml.Network] play.api.libs.json.JsResultException: JsResultException(errors:List((/name,List(ValidationError(List(error.maxLength),WrappedArray(63)), ValidationError(List(error.pattern),WrappedArray()))))) at play.api.libs.json.JsReadable$$anonfun$2.apply(JsReadable.scala:23) at play.api.libs.json.JsReadable$$anonfun$2.apply(JsReadable.scala:23) at play.api.libs.json.JsResult$class.fold(JsResult.scala:73) at play.api.libs.json.JsError.fold(JsResult.scala:13) at play.api.libs.json.JsReadable$class.as(JsReadable.scala:21) at play.api.libs.json.JsObject.as(JsValue.scala:76) ... 42 elided
Diff Detail
- Repository
- rMARATHON marathon
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Build has FAILED
Link to build: https://jenkins.mesosphere.com/service/jenkins/job/public-test-marathon-phabricator/852/
See console output for more information: https://jenkins.mesosphere.com/service/jenkins/job/public-test-marathon-phabricator/852/console
More generated code .. means less coverage?
[error] Coverage is below minimum [64.42% < 67.0%] java.lang.RuntimeException: Coverage minimum was not reached at scoverage.ScoverageSbtPlugin$.scoverage$ScoverageSbtPlugin$$checkCoverage(ScoverageSbtPlugin.scala:261) at scoverage.ScoverageSbtPlugin$$anonfun$coverageReport0$1.apply(ScoverageSbtPlugin.scala:125) at scoverage.ScoverageSbtPlugin$$anonfun$coverageReport0$1.apply(ScoverageSbtPlugin.scala:104) at scala.Function1$$anonfun$compose$1.apply(Function1.scala:47) at sbt.$tilde$greater$$anonfun$$u2219$1.apply(TypeFunctions.scala:40) at sbt.std.Transform$$anon$4.work(System.scala:63) at sbt.Execute$$anonfun$submit$1$$anonfun$apply$1.apply(Execute.scala:228) at sbt.Execute$$anonfun$submit$1$$anonfun$apply$1.apply(Execute.scala:228) at sbt.ErrorHandling$.wideConvert(ErrorHandling.scala:17) at sbt.Execute.work(Execute.scala:237) at sbt.Execute$$anonfun$submit$1.apply(Execute.scala:228) at sbt.Execute$$anonfun$submit$1.apply(Execute.scala:228) at sbt.ConcurrentRestrictions$$anon$4$$anonfun$1.apply(ConcurrentRestrictions.scala:159) at sbt.CompletionService$$anon$2.call(CompletionService.scala:28) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) at java.lang.Thread.run(Thread.java:745) [error] (marathon/*:coverageReport) Coverage minimum was not reached
Build has FAILED
Link to build: https://jenkins.mesosphere.com/service/jenkins/job/public-test-marathon-phabricator/856/
See console output for more information: https://jenkins.mesosphere.com/service/jenkins/job/public-test-marathon-phabricator/856/console
Bad ass. Seems like we can delete some of the wix validation now too?
More generated code is less coverage... no objections to lowering for this change.
Build has FAILED
Link to build: https://jenkins.mesosphere.com/service/jenkins/job/public-test-marathon-phabricator/860/
See console output for more information: https://jenkins.mesosphere.com/service/jenkins/job/public-test-marathon-phabricator/860/console
Build has FAILED
Link to build: https://jenkins.mesosphere.com/service/jenkins/job/public-test-marathon-phabricator/861/
See console output for more information: https://jenkins.mesosphere.com/service/jenkins/job/public-test-marathon-phabricator/861/console
Build has FAILED
Link to build: https://jenkins.mesosphere.com/service/jenkins/job/public-test-marathon-phabricator/873/
See console output for more information: https://jenkins.mesosphere.com/service/jenkins/job/public-test-marathon-phabricator/873/console
Build is green https://jenkins.mesosphere.com/service/jenkins/job/public-test-marathon-phabricator/875/ for more details.
several changes to the RAML spec were required because the serialized output was no longer readable (the new RAML-based validations failed against some serialized output)
one bugfix that we should probably backport to 1.4: PodInstanceStatus.instances.id should be string, not string.Name
src/test/scala/mesosphere/FutureTestSupport.scala | ||
---|---|---|
16 | without this change, running the tests individually and examining log output is very, very difficult (read: noisy) |
Bad ass. Seems like we can delete some of the wix validation now too?
More generated code is less coverage... no objections to lowering for this change.
src/test/scala/mesosphere/FutureTestSupport.scala | ||
---|---|---|
16 | Checking for the result every 2 seconds is a significant slowdown. Please make this much faster. |
Looks good - thanks!
build.sbt | ||
---|---|---|
154 ↗ | (On Diff #1383) | Does not have to be on that Patch: generated sources should not count into coverage. |
build.sbt | ||
---|---|---|
154 ↗ | (On Diff #1383) | They unfortunately do count |
build.sbt | ||
---|---|---|
154 ↗ | (On Diff #1383) | Did not look into the details, but you can exclude packages: https://github.com/scoverage/sbt-scoverage/blob/master/README.md#exclude-classes-and-packages |
build.sbt | ||
---|---|---|
154 ↗ | (On Diff #1383) | We have actual code in the same package though ;) Looks like the generator could use: // $COVERAGE-OFF$ at the top of every file though. |
build.sbt | ||
---|---|---|
154 ↗ | (On Diff #1383) | i'll give this a shot. better than artificially adjusting the expected coverage percentage |
src/test/scala/mesosphere/FutureTestSupport.scala | ||
16 | define "much faster"? where's the value threshold? 0.5 seconds? 1 second? yes, tests should run quickly but it should also be easy to debug them. the old defaults did not lend themselves well to debugging |
src/test/scala/mesosphere/FutureTestSupport.scala | ||
---|---|---|
16 | I see no harm in the result getting checked often. The timeout is long, it just checks often (milliseconds if I'm not mistaken) |
- use COVERAGE-OFF statement to subvert code coverage calculator w/ respect to RAML-generated scala files
Build has FAILED
Link to build: https://jenkins.mesosphere.com/service/jenkins/job/public-test-marathon-phabricator/903/
See console output for more information: https://jenkins.mesosphere.com/service/jenkins/job/public-test-marathon-phabricator/903/console
Build has FAILED
Link to build: https://jenkins.mesosphere.com/service/jenkins/job/public-test-marathon-phabricator/905/
See console output for more information: https://jenkins.mesosphere.com/service/jenkins/job/public-test-marathon-phabricator/905/console
Build is green https://jenkins.mesosphere.com/service/jenkins/job/public-test-marathon-phabricator/906/ for more details.
src/test/scala/mesosphere/FutureTestSupport.scala | ||
---|---|---|
16 | I changed this to 2s precisely because there IS harm in retries on the order of milliseconds - it significantly complicates debugging the tests. i've reviewed overall build and test times between this and prior builds (of other SHAs, unrelated to this diff). there is no significant difference in build/test times. the time to execute unit tests (which I run very often, locally) are not noticably impacted by this change (I see no difference on my machine). so if you think there's a "significant slowdown" please provide some numbers to back that up because I just don't see it in the numbers that I'm looking at |
Build is green https://jenkins.mesosphere.com/service/jenkins/job/public-test-marathon-phabricator/910/ for more details.
without this change, running the tests individually and examining log output is very, very difficult (read: noisy)