fix(@contract-app): fix contracts app tests #1982
Conversation
DeepCode Report (#e4ffae)DeepCode analyzed this pull request. |
| @@ -30,7 +33,8 @@ contract("AnotherStorage", function() { | |||
| assert.equal(result.toString(), SimpleStorage.options.address); | |||
| }); | |||
|
|
|||
| it("set ENS address", async function() { | |||
| // FIXME add back when the ENS feature is back | |||
| xit("set ENS address", async function() { | |||
There was a problem hiding this comment.
No, it's to disable the test. I did so because the ENS feature needs to be re-added to tests, but it's a whole different adventure that requires it's own PR
jrainville
force-pushed
the
fix/all-test-apps
branch
from
6eebaac
to
3c1c389
Compare
michaelsbradleyjr
force-pushed
the
fix/all-test-apps
branch
2 times, most recently
from
0edb153
to
d7ccd0d
Compare
These changes don't fix the race conditions related to the test dapp's tests but are a step in the right direction.
…sing Further refactoring is needed re: potentially duplicated or overlapping logic in `packages/plugins/solidity-tests` and `packages/core/utils/src/solidity/remapImports.ts`, as well in disabled test dapp tests
michaelsbradleyjr
force-pushed
the
fix/all-test-apps
branch
from
d7ccd0d
to
31ea24d
Compare
| deployIf: "await MyToken.methods.isAvailable().call()", | ||
| deployIf: (dependencies) => { | ||
| return dependencies.contract.MyToken.methods.isAvailable().call(); | ||
| }, |
There was a problem hiding this comment.
Did we turn off string support in deployment hooks? I'm generally all for it, but I don't see a reference as to why this change here is needed otherwise.
There was a problem hiding this comment.
No, it's just that we thought we had a race condition between the execution of the deployIfs/onDeploys and the execution of the tests, so changing the hooks to functions should have fixed that.
Sadly, it didn't have the wanted effect, but at least, the function syntax is nicer
| @@ -13,8 +13,11 @@ config({ | |||
| "SimpleStorage": { | |||
| args: [100] | |||
| }, | |||
| // "AnotherStorage": { | |||
| // args: ["$SimpleStorage", "embark.eth"] | |||
| // }, | |||
There was a problem hiding this comment.
If this is for the sake of keeping it around, this should probably be removed. Version control's got out back!
| "AnotherStorage": { | ||
| args: ["$SimpleStorage", "embark.eth"] | ||
| args: ["$SimpleStorage", "$SimpleStorage"] |
There was a problem hiding this comment.
This looks odd to me. Is injecting two SimpleStorage instances to AnotherStorage going to fix tests?
There was a problem hiding this comment.
No, that's a patch, because support for .eth addresses is currently broken. We need to add back the module support feature, which is a big task, that will need to come in another PR.
In the meantime, I pass two times the same address just for the sake of not modifying the contract
| @@ -12,6 +12,6 @@ contract OwnableTests { | |||
| } | |||
|
|
|||
| function shouldnotbezeroAddress() public { | |||
| Assert.equal(true, true, "owner is uninitialized"); | |||
| require(true == true, "owner is uninitialized"); | |||
There was a problem hiding this comment.
Why are we making this change?
There was a problem hiding this comment.
We had solidity errors because Assert. didn't exist.
We had forgotten that those are for the solidity tests. Eric reminded me yesterday. I'll need to revert and find why the remix tests don't work
They are failing because of a race condition; once that race condition has been fixed these tests should be reenabled.
Fix for the contract app