From e5fc12e256a1890352c7421fa18fcf523c818cb2 Mon Sep 17 00:00:00 2001 From: Pascal Precht Date: Mon, 27 May 2019 12:42:27 +0200 Subject: [PATCH] fix(@embark/test-runner): don't try to deploy and register ENS domains after JS tests have run When running unit tests inside a project that configures ENS subdomains using Smart Contract directives, the tests will output an error because of that particular Smart Contract's `deployedAddress` being `undefined`. This happens only when running tests, not when deploying the Smart Contracts including the custom ENS setup. It turns out that Embark attempts to compile and deploy the Smart Contracts of the project *twice* - once before tests are executed, and another time **after** tests are done executing. Both compilations/deployments are triggered through our custom `config()` function within test context, which ensures all Smart Contracts are deployed before tests are executed. This explains a few things: 1. There's no such issue when running `embark run`, in fact the custom ENS subdomains work perfectly fine 2. That's also why the tests are passing fine as well as the first compilation/deployment doesn't have any issues. The errors only appear *after* the tests have been executed. Still, this raises a few more questions, mainly - Why is the Smart Contract's `deployedAddress` property `undefined` when `config()` is executed a second time? - Why is `config()` executed a second time in the first place? Let's look into both of these. Assuming that there's a valid reason that `config()` is called twice, it's remains unclear why that property of a Smart Contract object is `undefined` in the second run. The reason for that is that Embark determines whether or not a particular Smart Contract should be deployed. One of the routines ensures that only the Smart Contracts configured for deployment are actually attempted to be deployed (as opposed to just deploying all Smart Contracts found in the file system). It turns out that the second `config()` call is done without any Smart Contract configuration, resulting basically in no deployment for any Smart Contract of the application. The `address` and `deployedAddress` of a Smart Contract object are however only set *after* deployment, resulting in them being `undefined`. **All this makes sense.** `config()` is simply not designed for being used with an empty `contracts: {}` config. This raises another question: Why is `config()` called with an empty configuration? This leads us to the second point. It does seem a bit weird that Embark tries to configure compile *and* deploy a DApp's Smart Contracts again right **after** the tests have been executed. So why is that? A quick `git blame` (no blame intended here) shows us that this routine has been introduced in https://github.com/embark-framework/embark/commit/12cbb7bdd. Notice the second list point of the commit: > Contracts that had been compiled in the JS tests were being deleted at the end of the JS test run, which caused errors > with the files not being found. The fix was to reset the contracts config to no longer require the non-test contracts > to be compiled/deployed. The above is a little tricky to understand without a little bit of context: Embark runs two types of tests, JavaScript tests and Solidity tests. It does that as part of a single process, keeping state in memory in-between those two test runs. With that in mind, the commit mentioned above says the following: 1. Artifacts that Embark generates as part of its compilation process are erased after JS tests are done executing. This makes a lot of sense as we don't want to leave any side effects undone when tests are finished. 2. The commit ensures that not only the artifacts are removed from disc, but also updates Embark's state in memory for `contractFiles`. The reason for that is that otherwise, in the second test run for Solidity tests, Embark will throw errors as it tries to look up the path for the artifacts in memory for all the Smart Contracts that had been compiled before. 3. Last but not least, there's *another* state that needs a reset and that's the Smart Contract configuration. If Embark doesn't reset the memory it'll assume in the second run that all of those Smart Contracts left in memory "have no code associated to it", while in reality, they shouldn't be in memory in the first place. So it seems that `config()` is called a second time with an empty Smart Contracts configuration just to ensure that memory is reset and no error messages are shown. As discussed earlier, this unfortunately also introduced a lot of side effects along the way as Embark tries to reregister ENS subdomains from Smart Contracts that are marked as undeployed and therefore don't have an address to interpolate. While there's probably different ways to go about it, the most straight forward fix is to simply not call `config()` a second time when it's really not needed. If the goal is to just reset the memory, we can take advantage of Embark's internal `config:contractsConfig:set` event, which is what this commit is doing. --- packages/embark-test-runner/src/index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/embark-test-runner/src/index.js b/packages/embark-test-runner/src/index.js index 4b86859fe9..e4e92bcb1c 100644 --- a/packages/embark-test-runner/src/index.js +++ b/packages/embark-test-runner/src/index.js @@ -240,8 +240,8 @@ class TestRunner { } let failures = runs.reduce((acc, val) => acc + val, 0); self.events.request('config:contractsFiles:reset', () => { - global.config({}, (err) => { - cb(err, {failures}); + self.events.request('config:contractsConfig:set', { contracts: {}}, () => { + cb(null, {failures}); }); }); });