Closed cunei closed 10 years ago
Toni, this is missing a test to check if building works, now. I'd like to see a test before merging. The testkept here does not validate your changes to my original patch do fix the issue, so I cannot give you a lgtm...
The fix appears better, but I don't want to accept any more patches that do not come with tests.
As I wrote in the notes to #128, the test that you wrote was not actually testing the failing case, which is why I have not included it initially on this branch. Indeed, the test was testing for something incorrect, and therefore failing against this correct fix. The empty lists of subprojects, which the test was using, are a symptom of an internal error, which is why I wrote the internal check that you wanted to remove.
I just completed the TRP smoke test against this pull request, and all works as intended. An updated integration test will follow.
A suitable integration test has been added. Also, the "temp-distributed-build-snapshots" repository has been re-added to the list of resolvers, since cunei/jacks is only published there at this time.
I like the new integration test. It looks like it should make it really quick/easy to start making tests. I would like to get better autoamted tests around the BuildSystem
interface itself, as the automated tests outlines the specification of inptus/outputs.
Particularly, it looks like I was misunderstanding the implied specifciation of this interface in my "fix". Glad you issues a real fix and we have an automated test. I would like to improve these automated tests over time, ensuring we abide by some higher-level specification for build systems.
A few minor comments, otherwise LGTM.
This commit includes the first automated test and other fixes from https://github.com/typesafehub/dbuild/pull/128, plus:
/cc @jsuereth