Refactor console-child.js to make the network settings more robust #5245
Conversation
There was a problem hiding this comment.
Looks good @sukanyaparashar. A few tests should be added for better coverage.
| describe("when run with options", () => { | ||
| it("displays the url hostname in the prompt", async () => { | ||
| const url = "http://localhost:8545"; | ||
| const parsedUrl = new URL(url); |
There was a problem hiding this comment.
These tests should cover the cases
- url
- runs in folder with a truffle-config
- runs in folder without a truffle-config
- network
- runs in folder with a truffle-config
There was a problem hiding this comment.
Sure, thanks for the suggestions @amal. I will add the tests to cover the mentioned cases.
| sandlot = tmp.dirSync({ unsafeCleanup: true }); | ||
| config = { working_directory: sandlot.name }; | ||
| config.logger = logger; | ||
| config.mocha = { |
There was a problem hiding this comment.
I'm not sure you need to configure config.mocha. Do the tests work with setting this value? I ask because I recently removed a bunch of these as the whole Reporter thing was unnecessary.
There was a problem hiding this comment.
Yeah! I just checked that it works without config.mocha. I will remove those.
| }; | ||
| }); | ||
| after(() => { | ||
| sandlot.removeCallback(); |
There was a problem hiding this comment.
You probably copied this from another test as well but you don't really need this as tmp cleans up after itself. Check out the "Synchronous file creation" section of this.
There was a problem hiding this comment.
Thanks for the link @eggplantzzz . It is clearly written that it will clean itself after. You are correct! Its not needed.
| config.logger = logger; | ||
| config.mocha = { | ||
| reporter: new Reporter(logger) |
There was a problem hiding this comment.
You might not need to set this property for all these tests. In other tests I've found this doesn't do anything and I was able to get rid of
config.mocha = {
reporter: new Reporter(logger)
}
There was a problem hiding this comment.
Yes, sure. I also checked removing those. And it seems that it doesn't do anything. I will get rid of these.
sukanyaparashar
force-pushed
the
fix-url-to-console
branch
from
564bbb1
to
d4eef2f
Compare
sukanyaparashar
force-pushed
the
fix-url-to-console
branch
2 times, most recently
from
d7d3836
to
d9da056
Compare
There was a problem hiding this comment.
Looks good to me, though @eggplantzzz may have more comments.
| network_id: "*", | ||
| url | ||
| }; | ||
| } else { | ||
| // Otherwise derive network settings | ||
| const customConfig = detectedConfig.networks.develop || {}; | ||
| const customConfig = detectedConfig.networks[network] || {}; |
There was a problem hiding this comment.
Should we use develop as a fallback? Because the settings in config.networks.develop are meant for the develop console. Or do we pass the name "develop" as a network in the case when it is the development console? Like maybe it should be
detectedConfig.networks[network] || detectedConfig.develop || {};
?
There was a problem hiding this comment.
We pass "develop" as a network in the console when using truffle develop.
ISSUE
PR #5087 broke
truffle console --networkbecause it hardcoded thenetwork nameto bedevelopThis was because
console-child.jshad logic exclusively used by thetruffle developcommand. Whentruffle console --network <network_name>is used, it adds thedevelopnetwork intoconfigwhich is not required. This behaviour doesn't make sense after PR #5087 becauseconsole-child.jshas to satisfy more code paths such as:truffle console --networktruffle console --urltruffle developSOLUTION
This PR makes
console-child.jsmore robust to handle the above cases. And also adds an integration test for thetruffle console --networkoption.NOTE: It needs to be merged before the next release so that it doesn't break the use of
--networkoptions with thetruffle consolecommand.