search

lightbend-labs / dbuild

Multi-project build tool, based on sbt.
https://lightbend-labs.github.io/dbuild
Other
83 stars 14 forks source link

First automated test of sbt-support. #128

Closed jsuereth closed 10 years ago

jsuereth commented 10 years ago

Still a lot of cleanup to do on the test itself, but it's a step in the right direction.

Review by @cunei and/or @eed3si9n

jsuereth commented 10 years ago

@cunei I fixed the errors I was seeing as well as some other tidbits I discovered as issues form the refactoring via the unit tests.

SINCE the unit tests now work, I'd like to also set up automated PR validation and prevent them from failing, so we dont' merge broken code again.

cunei commented 10 years ago

The pull request is more or less ok until 5335d5c, including half of cfdc337. However, I think you misidentified the problem related to the settings injection. The proposed fix in c0a2a2a patches some of the symptoms, but is not a complete fix; in the process, it introduces further breakage. Also, the integration test does not test the right thing.

So, the main symptom of this issue is a message like:

[info] [info] Loading project definition from /tmp/sbt_f0a49657/test-project/project/project
[info] com.fasterxml.jackson.databind.JsonMappingException: Required property 'in' is missing.
[info] {}
[info]  ^

The cause of the exception is the line at: https://github.com/typesafehub/dbuild/blob/ac4e1102a53b8ec39fd5b80e68ab50b044cded00/plugin/src/main/scala/com/typesafe/dbuild/plugin/DBuildRunner.scala#L700 where the 'in' property required by the RewireInput is missing from the rewireInputFile.

The proposed fix allows for an empty list of subprojects, in: https://github.com/typesafehub/dbuild/commit/c0a2a2a93c1cea51ae30c6456f7038c57005369a#diff-7988cf50ecfd9bc2df6c7879a39f75c0L79 and following lines. The list of subprojects, however, should be automatically computed during the extraction stage at each level; if it is empty, something must be wrong.

The change in: https://github.com/typesafehub/dbuild/commit/c0a2a2a93c1cea51ae30c6456f7038c57005369a#diff-7988cf50ecfd9bc2df6c7879a39f75c0L573 adds a further test on the list of subprojects. What is being tested here however, config.info.subproj, is not the list of subprojects for one level: subproj is a Seq[Seq[]], where each element refers to a separate level. If empty, that would mean that there are no levels, which is supposedly impossible.

The diff at: https://github.com/typesafehub/dbuild/commit/c0a2a2a93c1cea51ae30c6456f7038c57005369a#diff-895df7a8857bbf4844bf781b4c5267bfL104 replaces levels-1 with levels-2. That is not right: the number of .sbt files must be equal to the number of levels plus one. Compare with the explanation at: https://github.com/typesafehub/dbuild/blob/ac4e1102a53b8ec39fd5b80e68ab50b044cded00/support/src/main/scala/com/typesafe/dbuild/support/sbt/DependencyExtractor.scala#L42 and with the equivalent code at: https://github.com/typesafehub/dbuild/blob/ac4e1102a53b8ec39fd5b80e68ab50b044cded00/support/src/main/scala/com/typesafe/dbuild/support/sbt/DependencyExtractor.scala#L64

The patch at: https://github.com/typesafehub/dbuild/commit/c0a2a2a93c1cea51ae30c6456f7038c57005369a#diff-895df7a8857bbf4844bf781b4c5267bfL111 tries to generate a list of default BuildArtifactsIns, which are missing because of the discrepancy between the levels detected during extraction and those supplied in the additional sbt settings. The missing BuildArtifactsIn should have been instead automatically generated during the extraction stage. Indeed, inside each "project" directory there should be a ".dbuild" directory containing the RewireInput input file. Those files are missing, and that is the reason for the "{}" obtained while trying to rewire the additional levels.

In place of the proposed change, the proper fix is simply to let the extraction stage process the required number of levels; in other words, the line at: https://github.com/typesafehub/dbuild/blob/ac4e1102a53b8ec39fd5b80e68ab50b044cded00/support/src/main/scala/com/typesafe/dbuild/support/sbt/DependencyExtractor.scala#L51 should contain code similar to the one at: https://github.com/typesafehub/dbuild/blob/ac4e1102a53b8ec39fd5b80e68ab50b044cded00/support/src/main/scala/com/typesafe/dbuild/support/sbt/SbtBuildRunner.scala#L93 Further, the latter (as introduced in 4189f9e31e3011caad0cbda28ac68ebe0f5db1cd) can be simplified.

Once the calculation of levels for extraction and building is aligned, all works as intended. The SbtRewireSpec tests the wrong thing, in the sense that the (single) input files is supplied contains an empty list of subprojects, which instead is truly invalid and should be rejected. It does not test, as it should, that there is a project/.dbuild/rewire-input-data directory for each level, including the injected ones. Also see: https://github.com/typesafehub/dbuild/blob/ac4e1102a53b8ec39fd5b80e68ab50b044cded00/support/src/main/scala/com/typesafe/dbuild/support/sbt/SbtRunner.scala#L297

I am closing this pull request, and replacing it with a different one in which I keep the commits up to 5335d5c23dd02529c72ff8cf5502a82bc9f99a0b, plus the half of cfdc3372552facb3c52c4adc2e0dc40aa13fb06b which alters SbtTestHarness.scala. On top of that, I'll include a different fix, according to what has been described above.