Abstract cli response logging, and add test coverage. #348
Conversation
| @@ -24,17 +24,17 @@ export const handler = async (_argv: Arguments<Options>): Promise<void> => { | |||
| const { format } = _argv; | |||
|
|
|||
| if (format === "pretty") { | |||
| console.log(JSON.stringify(chains, null, 4)); | |||
| logger.log(JSON.stringify(chains, null, 4)); | |||
There was a problem hiding this comment.
Much of the code changes are simply replacing console.log with logger.log etc...
| if (!ens) { | ||
| logger.log( | ||
| "To use ENS, ensure you have set the enableEnsExperiment flag to true" | ||
| ); |
There was a problem hiding this comment.
These lines include a change to wrap with a try/catch, but also include an informative message in the case the user is trying to use ens without the required flags
| if (e.message.includes("in JSON at position")) { | ||
| console.log("Can't unwrap multiple rows. Use --unwrap=false"); | ||
| } catch (err: any) { | ||
| if (err.message.includes("in JSON at position")) { |
There was a problem hiding this comment.
We had a mix of caught errors being named e and err. Changing this to be consistent
| @@ -64,30 +65,23 @@ async function fireFullQuery( | |||
| const { type } = await globalThis.sqlparser.normalize(statement); | |||
| if (type !== "read" && !(await confirmQuery())) return; | |||
|
|
|||
| try { | |||
There was a problem hiding this comment.
There was a double try/catch block here, where both simply logged the error. Replaced them with a single try/catch
| error: function (message: string | unknown) { | ||
| console.error(message); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
This is the main change. The logger Object is a simple wrapper, which lets the tests spy on it instead of directly on the global console Object.
| name.endsWith(tableId) && | ||
| typeof transactionHash === "string" && | ||
| transactionHash.startsWith("0x") | ||
| ); |
There was a problem hiding this comment.
Using the calledWith sinon assertion was making it hard to determine why a test was failing. I replaced this kind of single assertion with multiple checks, with multiple equal assertions from the node.js core package.
This means when one of the assertions fails the test runner will let you know which assertion it was without needing to add logging or edit the tests.
joewagner
requested review from
carsonfarmer,
awmuncy,
sanderpick and
dtbuchholz
as code owners
| error: function (message: string | unknown) { | ||
| console.error(message); | ||
| }, | ||
| }; |
fixes tablelandnetwork/local-tableland#347
Summary
Two main changes are included here:
console.logto debug tests by wrapping calls with aloggerobject.Details
A lot of lines changed here, but the existing logic should all be preserved.
As well as making debugging via console.log possible, this will also enable us to turn on the local network logs to better understand what happened if a github test runner fails. This is particularly useful as we try to find a fix for tablelandnetwork/tableland-js#40
Code coverage is completing at 100%, but there are several
c8 ignorestatements. Most of these are either hard to mock error conditions or functionality that relates to the experimental ens feature.