From 9e8ba9ab88a109e3ed874e4b118e473a7768230a Mon Sep 17 00:00:00 2001 From: "Sean T. Allen" Date: Sun, 24 May 2020 14:02:30 -0400 Subject: [PATCH 1/3] Baby steps towards making CmdUpdate unit testable As part of the work on #120, an error when using the update command, I wanted to add tests to demonstrate the bug and then show that the bug was fixed by my changes. This is problematic because there are no real unit tests for any commands. This commit is the first step in providing better unit testability of various parts of corral in general and commands specifically. The only test added by this commit is that when using a corral.json with empty deps, that no VCS operations will be executed by _Updater. There are a number of changes that are brought in my this initial refactor. As we move forward with making corral more testable, some of these changes will probably fall away. I did what I considered the minimal thing to get a harness for testing _Updater working. This harness should be enough to test using the various test-data bundles that we have as well as testing issue #120. The approach I have taken does leave a lot of things in an inconsistent state. For example, this commit introduces a new way of organizing unit tests that keeps said tests near the code they are testing and allows for testing private implementations like the _Update that is part of the update cmd. These inconsistencies are temporary and as we move forward with adding more tests, will start to fall away. --- corral/cmd/_test_cmd_update.pony | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/corral/cmd/_test_cmd_update.pony b/corral/cmd/_test_cmd_update.pony index e2741a13..161b4103 100644 --- a/corral/cmd/_test_cmd_update.pony +++ b/corral/cmd/_test_cmd_update.pony @@ -143,4 +143,4 @@ primitive _TestData primitive _TestRepoCache fun apply(auth: AmbientAuth): FilePath ? => - FilePath(auth,"_test_cmd_update_repo_cache")? + FilePathS(auth,"_test_cmd_update_repo_cache")? From 6812825186e4cc6cb948e527311bba2d6b8e9210 Mon Sep 17 00:00:00 2001 From: "Sean T. Allen" Date: Mon, 25 May 2020 12:43:01 -0400 Subject: [PATCH 2/3] Add regression test for issue #120 --- corral/cmd/_test_cmd_update.pony | 40 +++++++++++++++++++ .../bundle-entrypoint/corral.json | 11 +++++ .../bundle-entrypoint/lock.json | 20 ++++++++++ .../regression-120/bundle-plain/corral.json | 4 ++ .../regression-120/bundle-plain/lock.json | 3 ++ .../bundle-transitive-sub-1/corral.json | 4 ++ .../bundle-transitive-sub-1/lock.json | 3 ++ .../bundle-transitive-sub-2/corral.json | 4 ++ .../bundle-transitive-sub-2/lock.json | 3 ++ .../bundle-transitive/corral.json | 11 +++++ .../bundle-transitive/lock.json | 10 +++++ corral/test/testdata/regression-120/lock.json | 10 +++++ 12 files changed, 123 insertions(+) create mode 100644 corral/test/testdata/regression-120/bundle-entrypoint/corral.json create mode 100644 corral/test/testdata/regression-120/bundle-entrypoint/lock.json create mode 100644 corral/test/testdata/regression-120/bundle-plain/corral.json create mode 100644 corral/test/testdata/regression-120/bundle-plain/lock.json create mode 100644 corral/test/testdata/regression-120/bundle-transitive-sub-1/corral.json create mode 100644 corral/test/testdata/regression-120/bundle-transitive-sub-1/lock.json create mode 100644 corral/test/testdata/regression-120/bundle-transitive-sub-2/corral.json create mode 100644 corral/test/testdata/regression-120/bundle-transitive-sub-2/lock.json create mode 100644 corral/test/testdata/regression-120/bundle-transitive/corral.json create mode 100644 corral/test/testdata/regression-120/bundle-transitive/lock.json create mode 100644 corral/test/testdata/regression-120/lock.json diff --git a/corral/cmd/_test_cmd_update.pony b/corral/cmd/_test_cmd_update.pony index 161b4103..f070ab8c 100644 --- a/corral/cmd/_test_cmd_update.pony +++ b/corral/cmd/_test_cmd_update.pony @@ -10,6 +10,8 @@ actor _TestCmdUpdate is TestList fun tag tests(test: PonyTest) => test(_TestEmptyDeps) + test(_TestRegression120) + class iso _TestEmptyDeps is UnitTest fun name(): String => @@ -37,6 +39,44 @@ class iso _TestEmptyDeps is UnitTest h.long_test(2_000_000_000) +class iso _TestRegression120 is UnitTest + fun name(): String => + "cmd/update/" + __loc.type_name() + + fun apply(h: TestHelper) ? => + """ + Issue #120 identified a problem with transitive dependencies that resulted + in too many operations being performaned across the loading of all + dependencies. + + The test as currently constituted, consists of a bundles with 2 + dependencies. One of those has 2 more in a transitive fashion, that should + result in 4 syncs and corresponding actions happening. However, due to a + bug in _Updater, it currently does 11. + + With a real VCS like Git, the number that results from it is variable + based on timing. This test exists to prove that issue #120 is fixed and + to prevent a similar bug from being introduced in the future. + """ + // given + let auth = h.env.root as AmbientAuth + let log = Log(LvlNone, h.env.err, SimpleLogFormatter) + let fp: FilePath = _TestData.file_path_from(h, "regression-120/bundle-entrypoint")? + let repo_cache = _TestRepoCache(auth)? + let ctx = Context(h.env, log, log, false, repo_cache) + let project = Project(auth, log, fp) + let bundle = Bundle.load(fp, log)? + let recorder = _OpsRecorder(h, 4, 4, 4) + let vcs_builder: VCSBuilder = _TestCmdUpdateVCSBuilder(recorder) + + // when + let updater = _Updater(ctx, project, consume bundle, vcs_builder, recorder) + + // when updater is finished, it will send a `cmd_completed` message to + // _OpsRecorder which will trigger pass/fail + h.long_test(2_000_000_000) + + actor _OpsRecorder is CmdResultReceiver let _h: TestHelper diff --git a/corral/test/testdata/regression-120/bundle-entrypoint/corral.json b/corral/test/testdata/regression-120/bundle-entrypoint/corral.json new file mode 100644 index 00000000..a2cc03e0 --- /dev/null +++ b/corral/test/testdata/regression-120/bundle-entrypoint/corral.json @@ -0,0 +1,11 @@ +{ + "deps": [ + { + "locator": "../bundle-transitive" + }, + { + "locator": "../bundle-plain" + } + ], + "info": {} +} diff --git a/corral/test/testdata/regression-120/bundle-entrypoint/lock.json b/corral/test/testdata/regression-120/bundle-entrypoint/lock.json new file mode 100644 index 00000000..2120ea03 --- /dev/null +++ b/corral/test/testdata/regression-120/bundle-entrypoint/lock.json @@ -0,0 +1,20 @@ +{ + "locks": [ + { + "locator": "../bundle-transitive-sub-2", + "revision": "master" + }, + { + "locator": "../bundle-transitive", + "revision": "master" + }, + { + "locator": "../bundle-plain", + "revision": "master" + }, + { + "locator": "../bundle-transitive-sub-1", + "revision": "master" + } + ] +} diff --git a/corral/test/testdata/regression-120/bundle-plain/corral.json b/corral/test/testdata/regression-120/bundle-plain/corral.json new file mode 100644 index 00000000..9734eb5e --- /dev/null +++ b/corral/test/testdata/regression-120/bundle-plain/corral.json @@ -0,0 +1,4 @@ +{ + "deps": [], + "info": {} +} diff --git a/corral/test/testdata/regression-120/bundle-plain/lock.json b/corral/test/testdata/regression-120/bundle-plain/lock.json new file mode 100644 index 00000000..944de670 --- /dev/null +++ b/corral/test/testdata/regression-120/bundle-plain/lock.json @@ -0,0 +1,3 @@ +{ + "locks": [] +} diff --git a/corral/test/testdata/regression-120/bundle-transitive-sub-1/corral.json b/corral/test/testdata/regression-120/bundle-transitive-sub-1/corral.json new file mode 100644 index 00000000..9734eb5e --- /dev/null +++ b/corral/test/testdata/regression-120/bundle-transitive-sub-1/corral.json @@ -0,0 +1,4 @@ +{ + "deps": [], + "info": {} +} diff --git a/corral/test/testdata/regression-120/bundle-transitive-sub-1/lock.json b/corral/test/testdata/regression-120/bundle-transitive-sub-1/lock.json new file mode 100644 index 00000000..944de670 --- /dev/null +++ b/corral/test/testdata/regression-120/bundle-transitive-sub-1/lock.json @@ -0,0 +1,3 @@ +{ + "locks": [] +} diff --git a/corral/test/testdata/regression-120/bundle-transitive-sub-2/corral.json b/corral/test/testdata/regression-120/bundle-transitive-sub-2/corral.json new file mode 100644 index 00000000..9734eb5e --- /dev/null +++ b/corral/test/testdata/regression-120/bundle-transitive-sub-2/corral.json @@ -0,0 +1,4 @@ +{ + "deps": [], + "info": {} +} diff --git a/corral/test/testdata/regression-120/bundle-transitive-sub-2/lock.json b/corral/test/testdata/regression-120/bundle-transitive-sub-2/lock.json new file mode 100644 index 00000000..944de670 --- /dev/null +++ b/corral/test/testdata/regression-120/bundle-transitive-sub-2/lock.json @@ -0,0 +1,3 @@ +{ + "locks": [] +} diff --git a/corral/test/testdata/regression-120/bundle-transitive/corral.json b/corral/test/testdata/regression-120/bundle-transitive/corral.json new file mode 100644 index 00000000..587cf601 --- /dev/null +++ b/corral/test/testdata/regression-120/bundle-transitive/corral.json @@ -0,0 +1,11 @@ +{ + "deps": [ + { + "locator": "../bundle-transitive-sub-2" + }, + { + "locator": "../bundle-transitive-sub-1" + } + ], + "info": {} +} diff --git a/corral/test/testdata/regression-120/bundle-transitive/lock.json b/corral/test/testdata/regression-120/bundle-transitive/lock.json new file mode 100644 index 00000000..a19d3fda --- /dev/null +++ b/corral/test/testdata/regression-120/bundle-transitive/lock.json @@ -0,0 +1,10 @@ +{ + "locks": [ + { + "locator": "../bundle-transitive-sub-2" + }, + { + "locator": "../bundle-transitive-sub-1/" + } + ] +} diff --git a/corral/test/testdata/regression-120/lock.json b/corral/test/testdata/regression-120/lock.json new file mode 100644 index 00000000..476468b3 --- /dev/null +++ b/corral/test/testdata/regression-120/lock.json @@ -0,0 +1,10 @@ +{ + "locks": [ + { + "locator": "bundle-transitive" + }, + { + "locator": "bundle-plain" + } + ] +} From d17741e81d26e4cbe448d95e9c8fa350a9eaf0b3 Mon Sep 17 00:00:00 2001 From: "Sean T. Allen" Date: Sun, 7 Jun 2020 06:42:32 -0400 Subject: [PATCH 3/3] Fix for #120 It's very ugly and there's a lot of design work that needs to do. --- corral/cmd/_test_cmd_update.pony | 2 +- corral/cmd/cmd_update.pony | 18 ++++++++++++------ 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/corral/cmd/_test_cmd_update.pony b/corral/cmd/_test_cmd_update.pony index f070ab8c..5f23eb1a 100644 --- a/corral/cmd/_test_cmd_update.pony +++ b/corral/cmd/_test_cmd_update.pony @@ -183,4 +183,4 @@ primitive _TestData primitive _TestRepoCache fun apply(auth: AmbientAuth): FilePath ? => - FilePathS(auth,"_test_cmd_update_repo_cache")? + FilePath(auth,"_test_cmd_update_repo_cache")? diff --git a/corral/cmd/cmd_update.pony b/corral/cmd/cmd_update.pony index e9d862bf..5b661152 100644 --- a/corral/cmd/cmd_update.pony +++ b/corral/cmd/cmd_update.pony @@ -30,6 +30,7 @@ actor _Updater let deps_seen: Map[Locator, Dep] ref = deps_seen.create() let deps_to_load: Map[Locator, Dep] ref = deps_to_load.create() + let deps_loading: Map[Locator, Dep] ref = deps_loading.create() let dep_tags: Map[Locator, Array[String] val] ref = dep_tags.create() let _vcs_builder: VCSBuilder @@ -67,18 +68,20 @@ actor _Updater fun ref load_queued_deps() => for dep in deps_to_load.values() do - try - load_dep(dep)? - else - ctx.uout.err("Error loading dep " + dep.name()) - // It won't get a lock. How should we handle/report the error? + if not deps_loading.contains(dep.locator) then + try + load_dep(dep)? + else + ctx.uout.err("Error loading dep " + dep.name()) + // It won't get a lock. How should we handle/report the error? + end end end if deps_to_load.size() == 0 then _results_receiver.cmd_completed() end - fun load_dep(dep: Dep) ? => + fun ref load_dep(dep: Dep) ? => let vcs = _vcs_builder(dep.vcs())? let repo = RepoForDep(ctx, project, dep)? @@ -109,6 +112,8 @@ actor _Updater let sync_op = vcs.sync_op(sync_handler) sync_op(repo) + deps_loading(locator) = dep + be load_transitive_dep(locator: Locator) => ctx.log.info("Loading transitive dep: " + locator.path()) try @@ -127,6 +132,7 @@ actor _Updater try // If this remove fails, it just means another path got here first. (_, let dep) = deps_to_load.remove(locator)? + deps_loading.remove(locator)? ctx.log.fine("tags for " + dep.locator.string() + ": " + tags.size().string()) dep_tags(locator) = tags end