Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Internal improvement: refactor connectOrStart #4671

Merged
merged 9 commits into from
May 2, 2022
3 changes: 3 additions & 0 deletions packages/environment/.eslintrc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"extends": ["../../.eslintrc.package.json"]
}
13 changes: 6 additions & 7 deletions packages/environment/chain.js
Original file line number Diff line number Diff line change
Expand Up @@ -227,14 +227,13 @@ class GanacheMixin {

// cleanup Ganache process on exit
exit(_supervisor) {
this.ganache.close(err => {
if (err) {
this.ganache
.close()
.then(() => (process.exitCode = 0))
.catch(err => {
console.error(err.stack || err);
process.exit(1);
} else {
process.exit();
}
});
process.exitCode = 1;
});
}
}

Expand Down
38 changes: 17 additions & 21 deletions packages/environment/develop.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const { spawn } = require("child_process");
const debug = require("debug");

const Develop = {
start: async function(ipcNetwork, options = {}) {
start: async function (ipcNetwork, options = {}) {
let chainPath;

// The path to the dev env process depends on whether or not
Expand Down Expand Up @@ -39,7 +39,7 @@ const Develop = {
});
},

connect: function(options) {
connect: function (options) {
const debugServer = debug("develop:ipc:server");
const debugClient = debug("develop:ipc:client");
const debugRPC = debug("develop:ganache");
Expand Down Expand Up @@ -69,7 +69,7 @@ const Develop = {
if (options.log) {
debugRPC.enabled = true;

loggers.ganache = function() {
loggers.ganache = function () {
// HACK-y: replace `{}` that is getting logged instead of ""
var args = Array.prototype.slice.call(arguments);
if (
Expand All @@ -88,21 +88,21 @@ const Develop = {
ipc.config.maxRetries = 0;
}

var disconnect = function() {
var disconnect = function () {
ipc.disconnect(ipcNetwork);
};

return new Promise((resolve, reject) => {
ipc.connectTo(ipcNetwork, connectPath, function() {
ipc.of[ipcNetwork].on("destroy", function() {
ipc.connectTo(ipcNetwork, connectPath, function () {
ipc.of[ipcNetwork].on("destroy", function () {
reject(new Error("IPC connection destroyed"));
});

ipc.of[ipcNetwork].on("truffle.ready", function() {
ipc.of[ipcNetwork].on("truffle.ready", function () {
resolve(disconnect);
});

Object.keys(loggers).forEach(function(key) {
Object.keys(loggers).forEach(function (key) {
var log = loggers[key];
if (log) {
var message = `truffle.${key}.log`;
Expand All @@ -113,29 +113,25 @@ const Develop = {
});
},

connectOrStart: async function(options, ganacheOptions) {
connectOrStart: async function (options, ganacheOptions) {
options.retry = false;

const ipcNetwork = options.network || "develop";

let connectedAlready = false;
let started = false;
let disconnect;

try {
const disconnect = await this.connect(options);
connectedAlready = true;
return {
started: false,
disconnect
};
disconnect = await this.connect(options);
} catch (_error) {
await this.start(ipcNetwork, ganacheOptions);
options.retry = true;
const disconnect = await this.connect(options);
if (connectedAlready) return;
connectedAlready = true;
disconnect = await this.connect(options);
started = true;
} finally {
return {
started: true,
disconnect
disconnect,
started
};
}
}
Expand Down
57 changes: 57 additions & 0 deletions packages/environment/test/connectOrStartTest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
const assert = require("chai").assert;
const Develop = require("../develop");

describe("connectOrStart test network", async function () {
const ipcOptions = { network: "test" };
const ganacheOptions = {
host: "127.0.0.1",
network_id: 666,
port: 6969
};

it("starts Ganache when no Ganache instance is running", async function () {
let connection;
try {
connection = await Develop.connectOrStart(ipcOptions, ganacheOptions);
assert.isTrue(connection.started, "A new Ganache server did not spin up");
assert.isFunction(connection.disconnect, "disconnect is not a function");
} finally {
if (connection) {
connection.disconnect();
}
}
});

it("connects to an established Ganache instance", async function () {
let connectionOneDisconnect, connectionTwo;
let spawnedGanache;

try {
//Establish IPC Ganache service
spawnedGanache = await Develop.start(ipcOptions.network, ganacheOptions);
connectionOneDisconnect = await Develop.connect({
...ipcOptions,
retry: true
});

//Test
connectionTwo = await Develop.connectOrStart(ipcOptions, ganacheOptions);

//Validate
assert.isFalse(connectionTwo.started);
} finally {
//Cleanup IPC2
if (connectionTwo.disconnect) {
connectionTwo.disconnect();
}
//Cleanup IPC1
if (connectionOneDisconnect) {
connectionOneDisconnect();
}
//Signal Ganache Process to exit
process.kill(spawnedGanache.pid, "SIGHUP");
//Resolve on confirmation of process exit
await new Promise(resolve => spawnedGanache.on("exit", () => resolve()));
}
});
});
16 changes: 8 additions & 8 deletions packages/environment/test/develop.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,15 @@ describe("Environment develop", function () {
gas: expectedNetwork.gas
}
};
sinon.stub(Environment, "detect");
});

afterEach("Restore Environment detect", async function () {
Environment.detect.restore();
});

it("Environment.develop overwrites the network_id of the network", async function() {
it("Environment.develop overwrites the network_id of the network", async function () {
//Stub detect in order to prevent its executing during this test
//which is invoked during `Environment.develop(..)`
sinon.stub(Environment, "detect");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused why this is being stubbed... what is this for?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Environment.develop() invokes Environment.detect() as its final step, which isn't needed to validate the test's assertion. I'll add a clarifying comment and leave potential refactoring for followup work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, huh, OK. Odd use and I'd suggest getting rid of it, but I guess it's OK to put that off?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of those things that touches TruffleConfig's side-effect behavior. There are a bunch of methods in Environment that modify TruffleConfig :/ . A code smell to be addressed later.

await Environment.develop(config, testOptions);
const mutatedNetwork = config.networks[config.network];

const mutatedNetwork = config.networks[config.network];
assert.equal(
mutatedNetwork.port,
expectedNetwork.port,
Expand All @@ -55,6 +53,8 @@ describe("Environment develop", function () {
expectedNetwork.gas,
"The gas of the network should be unchanged."
);
});

//Restore `detect` functionality
Environment.detect.restore();
});
}).timeout(10000);