Add code coverage tooling #45

jackson-at-bentley · 2022-06-24T20:57:58Z

npm run test:standalone

I chose nyc because other projects at Bentley like ecschema2ts use it. If I configured everything correctly, the synchronizer has about 40 percent line coverage.

This is a breaking PR, but I don't think any projects depend on the v3 @itwin/connector-framework yet.

I merged my branch as-a-user which passes the connector class object to the connector runner. I wasn't able to get the tests to run otherwise. This indirectly resolves ConnectorRunner should not invoke require in loadConnector #40. The tests failed to open the path given to require. I really have no idea what's going on, but I suspect that because nyc depends on ts-node for JIT complication it wasn't building the connector TypeScript.
I installed mocha-suppress-logs to suppress the Logger output.
I rearranged the project structure to make mocha happy. test is now in the root directory.
I added a Coveralls GitHub workflow.

To address

The HTML reporter fails to correctly link to the source files. I've opened an issue on nyc, but the text reporter works as shown above.
npm run test:connector is failing with fetch is not defined. That code hasn't changed, and isn't covered by the tests [Figure 1].
npm run test:integration is failing only on Linux with Failed OIDC signin. Looks like something is not right. Please contact your administrator. 400 - Invalid client_id. There's this very suspicious comment in oicd-signin-tool [Figure 2]. This also happens on the main branch on Linux.
We shouldn't need to compile everything in the test directory because of the ts-node JIT tooling, but I'm not sure how else to configure nyc.

I've never configured coverage tooling before so I welcome any and all feedback.

What's next

I'd like to add coverage standards to our integration, and maybe a service like coveralls for visibility.
I think we really need an import style standard.

Figures

Figure 1

Figure 2

// Launch puppeteer with no sandbox only on linux
let launchOptions = { dumpio: true }; // , headless: false, slowMo: 500 };
if (os.platform() === "linux") {
    launchOptions = {
        args: ["--no-sandbox"], // , "--disable-setuid-sandbox"],
    };
}

test/integration/ConnectorRunner.test.ts

src/ConnectorRunner.ts

jackson-at-bentley

Annotated.

jackson-at-bentley

A few miscellaneous changes.

jackson-at-bentley · 2022-06-27T15:33:01Z

package.json

@@ -56,6 +54,7 @@
    "@types/mocha": "^8.2.2",
    "@types/node": "^14.0.0",
    "@types/object-hash": "^1.3.0",
+    "@types/ws": "^8.5.3",


This repository still won't build if linked externally. This import resolves the missing ws types that core-backend version 3.2.4 depends on.

node_modules/@itwin/core-backend/lib/cjs/LocalhostIpcHost.d.ts:4:21 - error TS7016: Could not find a declaration file for module 'ws'. '/home/jackson/bentley/connector-demo/node_modules/ws/index.js' implicitly has an 'any' type. Try `npm i --save-dev @types/ws` if it exists or add a new declaration (.d.ts) file containing `declare module 'ws';` 4 import * as ws from "ws";

jackson-at-bentley · 2022-06-27T15:33:32Z

tsconfig.json

@@ -4,11 +4,9 @@
    "target": "es2017",
    "module": "commonjs",
    "moduleResolution": "node",
-    "outDir": "./lib"
+    "outDir": "./lib",
+    "strict": true,


I think we should be using strict mode.

akshay-madhukar

Why is this a breaking change? Adding code coverage tooling shouldn't change the exported members right?

jackson-at-bentley · 2022-06-28T18:25:22Z

Why is this a breaking change? Adding code coverage tooling shouldn't change the exported members right?

It changes the API for the connector runner.

jchick-bentley · 2022-07-05T23:15:06Z

src/ConnectorRunner.ts

@@ -352,10 +352,8 @@ export class ConnectorRunner {

  private initProgressMeter() {}

-  private async loadConnector(connectorFile: string) {
-    // eslint-disable-next-line @typescript-eslint/no-var-requires
-    const connectorClass = require(connectorFile).default;


@jffmarker @akshay-madhukar @admccarthy1 Jackson did a lot of refactoring around this connectorFile: string to avoid the require call (see ConnectorRunner.ts line 357). We looked through nodeApp.ts and see the code is presently set up to read the sourcePath from environment variables and set this as one of the jobArgs. So, it seems that if we approve this change, we will have to move the require call (or something equivalent) upstream to nodeApp.ts. Seems like a big change at this stage. Is there benefit to moving this out to NodeApp.Ts? Are there other orchestration workflows that also expect to pass a relative path to the .js connector source? These would also need to be updated to do the require call as well.

jackson-at-bentley and others added 5 commits June 20, 2022 11:31

Export Args and do not use require in ConnectorRunner

bbe6663

Pass connector class object to runner

415ad56

Replace Windows path with join from node:path

3efc81b

Begin to integrate nyc for coverage

15a5089

ConnectorRunner should not invoke require

f9b24ba

jackson-at-bentley requested review from abeesh, admccarthy1, akshay-madhukar, jchick-bentley, jffmarker and swwilson-bsi as code owners June 24, 2022 20:57

jackson-at-bentley commented Jun 24, 2022

View reviewed changes

test/integration/ConnectorRunner.test.ts Outdated Show resolved Hide resolved

jackson-at-bentley commented Jun 24, 2022

View reviewed changes

src/ConnectorRunner.ts Outdated Show resolved Hide resolved

jackson-at-bentley commented Jun 24, 2022

View reviewed changes

Fix argument to runner in standalone test

c3013dd

jackson-at-bentley commented Jun 27, 2022

View reviewed changes

jackson-at-bentley added 4 commits June 27, 2022 09:17

Fix types export in package.json

73ffdd2

Remove .env hack, configured by dotenv per process

9eac806

Fix connector error path

b6aa1d3

Restore log suppression and downgrade ts-node

1d900fe

akshay-madhukar reviewed Jun 28, 2022

View reviewed changes

Standalone iModels use briefcases

f35a50f

jackson-at-bentley mentioned this pull request Jun 29, 2022

Cover synchronizer #46

Closed

jackson-at-bentley added 2 commits July 5, 2022 11:11

Add Coveralls GitHub workflow

782220a

Remove README coverage badge for local fork

7904630

jchick-bentley reviewed Jul 5, 2022

View reviewed changes

jackson-at-bentley added 3 commits July 8, 2022 09:34

Regress runner argument, one test still failing

aba33dd

Revert package-lock.json, test still failing

34d35db

Document regression

0244a8e

Merge into main

2a4c71f

jackson-at-bentley mentioned this pull request Jul 11, 2022

Add code coverage tooling and Coveralls workflow #53

Merged

5 tasks

jackson-at-bentley closed this Jul 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add code coverage tooling #45

Add code coverage tooling #45

jackson-at-bentley commented Jun 24, 2022 •

edited

Loading

jackson-at-bentley left a comment

jackson-at-bentley left a comment

jackson-at-bentley Jun 27, 2022

jackson-at-bentley Jun 27, 2022

akshay-madhukar left a comment

jackson-at-bentley commented Jun 28, 2022

jchick-bentley Jul 5, 2022

Add code coverage tooling #45

Add code coverage tooling #45

Conversation

jackson-at-bentley commented Jun 24, 2022 • edited Loading

npm run test:standalone

To address

What's next

Figures

jackson-at-bentley left a comment

Choose a reason for hiding this comment

jackson-at-bentley left a comment

Choose a reason for hiding this comment

jackson-at-bentley Jun 27, 2022

Choose a reason for hiding this comment

jackson-at-bentley Jun 27, 2022

Choose a reason for hiding this comment

akshay-madhukar left a comment

Choose a reason for hiding this comment

jackson-at-bentley commented Jun 28, 2022

jchick-bentley Jul 5, 2022

Choose a reason for hiding this comment

jackson-at-bentley commented Jun 24, 2022 •

edited

Loading