Internal improvement: refactor connectOrStart #4671

cds-amal · 2022-01-28T02:56:34Z

This PR

updates the use of the Ganache's server.close() method, which uses a promise instead of node's callback idiom. We should tear down Ganache properly.
cleans up the logic for connectOrStart
adds 2 tests to verify connectOfStart try/catch logic path.

haltman-at

Seems basically good to me, confused about something in .eslintrc.json though.

packages/environment/.eslintrc.json

packages/environment/develop.js

packages/environment/test/connectOrStartTest.js

cds-amal · 2022-01-29T20:41:58Z

This PR continues to be dogged by some race condition(s) that prevents CI from logging completed steps. What a PITA.

Cancelling the job forces a flush, it seems.

@truffle/environment: connectOrStart test network
@truffle/environment: ✓ starts Ganache when no Ganache instance is running (1561ms)
@truffle/environment: ✓ connects to an established Ganache instance (1056ms)

Ah, this PR's tests pass, but did it break another test? 🤔

@truffle/environment: Environment develop
@truffle/environment: - Preparing to download box
@truffle/environment: ✔ Preparing to download box
@truffle/environment: - Downloading
@truffle/environment: ✖ Downloading
@truffle/environment: Unbox failed!
@truffle/environment: ✖ Downloading
@truffle/environment: Unbox failed!
@truffle/environment: 1) "before each" hook: Create a sandbox for "Environment.develop overwrites the network_id of the network"
@truffle/environment: 2) "after each" hook: Restore Environment detect for "Environment.develop overwrites the network_id of the network"
@truffle/environment: 2 passing (4s)
@truffle/environment: 2 failing
@truffle/environment: 1) Environment develop
@truffle/environment: "before each" hook: Create a sandbox for "Environment.develop overwrites the network_id of the network":
@truffle/environment: Error: Error connecting to https://raw.githubusercontent.com/trufflesuite/truffle-init-default/master/truffle-box.json. Please check your internet connection and try again.
@truffle/environment: Request failed with status code 500
@truffle/environment: at createError (/home/runner/work/truffle/truffle/node_modules/axios/lib/core/createError.js:16:15)
@truffle/environment: at settle (/home/runner/work/truffle/truffle/node_modules/axios/lib/core/settle.js:17:12)
@truffle/environment: at IncomingMessage.handleStreamEnd (/home/runner/work/truffle/truffle/node_modules/axios/lib/adapters/http.js:269:11)
@truffle/environment: at endReadableNT (_stream_readable.js:1241:12)
@truffle/environment: at processTicksAndRejections (internal/process/task_queues.js:84:21)
@truffle/environment: 2) Environment develop
@truffle/environment: "after each" hook: Restore Environment detect for "Environment.develop overwrites the network_id of the network":
@truffle/environment: TypeError: Environment.detect.restore is not a function
@truffle/environment: at Context. (test/develop.js:36:24)
@truffle/environment: at processImmediate (internal/timers.js:461:21)
Error: The operation was canceled.

haltman-at · 2022-01-31T20:17:14Z

So, wait, is this working now, or was that just luck?

cds-amal · 2022-02-01T00:01:23Z

Hard to tell @haltman-at. I'm going to wait on the fix to Ganache-2185 as it seems to be relevant during my investigation.

cds-amal · 2022-02-01T19:00:22Z

Yep, definitely lucky the last time. I re-ran CI and it failed unable to shut down Ganache, killing the test-runner that needed to be manually cancelled.

@truffle/environment: ✔ Setting up box
@truffle/environment:     ✓ Environment.develop overwrites the network_id of the network
@truffle/environment:   2 passing (3s)
@truffle/environment:   1 failing
@truffle/environment:   1) connectOrStart test network
@truffle/environment:        connects to an established Ganache instance:
@truffle/environment:      Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/truffle/truffle/packages/environment/test/connectOrStartTest.js)
@truffle/environment:       at listOnTimeout (internal/timers.js:554:17)
@truffle/environment:       at processTimers (internal/timers.js:497:7)
Error: The operation was canceled.

packages/environment/test/connectOrStartTest.js

haltman-at · 2022-04-28T17:16:23Z

Oh hey a quick note, the .eslintrc.json changes are no longer necessary as of #4698; I'd suggest rebasing if you haven't and removing that change because it's redundant.

cds-amal · 2022-04-28T18:42:29Z

Oh hey a quick note, the .eslintrc.json changes are no longer necessary as of #4698; I'd suggest rebasing if you haven't and removing that change because it's redundant.

Good catch! rebasing.

haltman-at

Seems mostly good to me (as best I can understand anyway); just two questions about the tests.

packages/environment/test/develop.js

haltman-at · 2022-04-28T18:53:58Z

packages/environment/test/develop.js

  });

-  it("Environment.develop overwrites the network_id of the network", async function() {
+  it("Environment.develop overwrites the network_id of the network", async function () {
+    sinon.stub(Environment, "detect");


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

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.

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

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.

This function [1] changed and returns a promise and does not accept a callback. 1: https://github.com/trufflesuite/ganache/blob/d5c3bcbcf2198d2664bd9bfafd331d80994afc7f/src/packages/core/src/server.ts#L272

Test to validate code path to connect to an established Ganache server, or no Ganache server.

- remove catch - add messages to assertions

set instead of invoking , which forces the process to exit as quickly as possible even if there are still asynchronous operations pending that have not yet completed fully, including I/O operations to process.stdout and process.stderr.

eggplantzzz

lookin good

haltman-at

OK, looks good to me!

cds-amal marked this pull request as draft January 28, 2022 02:56

cds-amal force-pushed the connect-or-start branch from 53aeb32 to 4c92148 Compare January 28, 2022 19:37

cds-amal marked this pull request as ready for review January 28, 2022 19:59

cds-amal requested review from eggplantzzz, davidmurdoch, dkillen, fainashalts, gnidan, haltman-at, kevinbluer, kevinweaver, leeftk, lsqproduction, sukanyaparashar and tenthirtyone January 28, 2022 20:01

haltman-at reviewed Jan 28, 2022

View reviewed changes

packages/environment/.eslintrc.json Outdated Show resolved Hide resolved

packages/environment/develop.js Show resolved Hide resolved

davidmurdoch reviewed Jan 28, 2022

View reviewed changes

packages/environment/test/connectOrStartTest.js Outdated Show resolved Hide resolved

davidmurdoch reviewed Jan 29, 2022

View reviewed changes

packages/environment/test/connectOrStartTest.js Outdated Show resolved Hide resolved

cds-amal self-assigned this Feb 3, 2022

This was referenced Feb 3, 2022

Core: add managed ganache version info to the output of truffle version #4686

Merged

Internal improvement: Put all BUNDLE globals in main .eslintrc.json #4698

Merged

cds-amal force-pushed the connect-or-start branch from 2c713aa to 72fb420 Compare March 9, 2022 16:21

cds-amal force-pushed the connect-or-start branch from 72fb420 to 16d4633 Compare April 28, 2022 12:33

cds-amal requested a review from haltman-at April 28, 2022 13:08

lsqproduction reviewed Apr 28, 2022

View reviewed changes

packages/environment/test/connectOrStartTest.js Outdated Show resolved Hide resolved

cds-amal force-pushed the connect-or-start branch from 16d4633 to 787a595 Compare April 28, 2022 18:41

haltman-at reviewed Apr 28, 2022

View reviewed changes

cds-amal force-pushed the connect-or-start branch from a2724f7 to c108f02 Compare May 1, 2022 04:24

cds-amal added 9 commits May 1, 2022 11:21

environment: cleanup connect or start logic

62aba0e

environment: update ganache.server.close

f360e1f

This function [1] changed and returns a promise and does not accept a callback. 1: https://github.com/trufflesuite/ganache/blob/d5c3bcbcf2198d2664bd9bfafd331d80994afc7f/src/packages/core/src/server.ts#L272

environment: add connectOrStart tests

6c9d67b

Test to validate code path to connect to an established Ganache server, or no Ganache server.

environment: use finally for cleanup

17c0108

environment: allow exceptions to bubble up

2729294

- remove catch - add messages to assertions

environment: give chain process time to clean-up

8b08817

set instead of invoking , which forces the process to exit as quickly as possible even if there are still asynchronous operations pending that have not yet completed fully, including I/O operations to process.stdout and process.stderr.

environment: add mocha eslint definitions

f57ba33

environment: rework test setup and teardown

076d009

environment: refactor and comment a test

89f59ad

cds-amal force-pushed the connect-or-start branch from bc95749 to 89f59ad Compare May 1, 2022 15:24

eggplantzzz approved these changes May 2, 2022

View reviewed changes

haltman-at approved these changes May 2, 2022

View reviewed changes

cds-amal merged commit 446efc6 into develop May 2, 2022

cds-amal deleted the connect-or-start branch May 2, 2022 23:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Internal improvement: refactor connectOrStart #4671

Internal improvement: refactor connectOrStart #4671

cds-amal commented Jan 28, 2022 •

edited

Loading

haltman-at left a comment

cds-amal commented Jan 29, 2022 •

edited

Loading

haltman-at commented Jan 31, 2022

cds-amal commented Feb 1, 2022

cds-amal commented Feb 1, 2022

haltman-at commented Apr 28, 2022

cds-amal commented Apr 28, 2022

haltman-at left a comment

haltman-at Apr 28, 2022

cds-amal May 1, 2022

haltman-at May 2, 2022

cds-amal May 2, 2022

eggplantzzz left a comment

haltman-at left a comment

Internal improvement: refactor connectOrStart #4671

Internal improvement: refactor connectOrStart #4671

Conversation

cds-amal commented Jan 28, 2022 • edited Loading

haltman-at left a comment

Choose a reason for hiding this comment

cds-amal commented Jan 29, 2022 • edited Loading

haltman-at commented Jan 31, 2022

cds-amal commented Feb 1, 2022

cds-amal commented Feb 1, 2022

haltman-at commented Apr 28, 2022

cds-amal commented Apr 28, 2022

haltman-at left a comment

Choose a reason for hiding this comment

haltman-at Apr 28, 2022

Choose a reason for hiding this comment

cds-amal May 1, 2022

Choose a reason for hiding this comment

haltman-at May 2, 2022

Choose a reason for hiding this comment

cds-amal May 2, 2022

Choose a reason for hiding this comment

eggplantzzz left a comment

Choose a reason for hiding this comment

haltman-at left a comment

Choose a reason for hiding this comment

cds-amal commented Jan 28, 2022 •

edited

Loading

cds-amal commented Jan 29, 2022 •

edited

Loading