Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add code coverage tooling #45

Closed
wants to merge 17 commits into from
Closed

Add code coverage tooling #45

wants to merge 17 commits into from

Conversation

jackson-at-bentley
Copy link
Contributor

@jackson-at-bentley jackson-at-bentley commented Jun 24, 2022

npm run test:standalone

coverage

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

fetch

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"],
    };
}

Copy link
Contributor Author

@jackson-at-bentley jackson-at-bentley left a comment

Choose a reason for hiding this comment

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

Annotated.

Copy link
Contributor Author

@jackson-at-bentley jackson-at-bentley left a comment

Choose a reason for hiding this comment

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

A few miscellaneous changes.

@@ -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",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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";

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

Choose a reason for hiding this comment

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

I think we should be using strict mode.

Copy link
Collaborator

@akshay-madhukar akshay-madhukar left a comment

Choose a reason for hiding this comment

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

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

@jackson-at-bentley
Copy link
Contributor Author

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.

@@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ConnectorRunner should not invoke require in loadConnector
3 participants