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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add code coverage tooling and Coveralls workflow #53

Merged
merged 18 commits into from Jul 12, 2022
Merged

Conversation

jackson-at-bentley
Copy link
Contributor

@jackson-at-bentley jackson-at-bentley commented Jul 11, 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've reverted the breaking API change in #45, which I'm closing because I can't run the Azure pipeline on the fork. My "fix" is hardcoding the path to the test connector's schema, because __dirname's value in KnownTestLocations changes when running JIT with ts-node and when being require'ed.

  • 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. If Jason is okay with giving me the keys 馃攽 to the repository for a day I'd like to finish setting it up (authenticating with Coveralls and adding the Coveralls account as a collaborator so it can leave coverage comments).

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. (I've downgraded ts-node.)
  • 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, because parts of that directory need to be compiled.

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.

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
Collaborator

@jchick-bentley jchick-bentley left a comment

Choose a reason for hiding this comment

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

@jackson-at-bentley can you please elaborate on "Jason is okay with giving me the keys key to the repository for a day I'd like to finish setting it up (authenticating with Coveralls and adding the Coveralls account as a collaborator so it can leave coverage comments)." ? Feel free to provide links to relevant coveralls documentation. By keys to repo, it sounds like you can't do what is necessary via pull request or your current (write) access?

@jackson-at-bentley
Copy link
Contributor Author

@jchick-bentley I was enjoying the imagery of joyriding the repository around. Here's the steps I took on my fork to get Coveralls working.

  1. Sign us up for Coveralls.
  2. Follow the steps in the GitHub actions README.
    1. You have invited the @coveralls user to your repository as a collaborator
    2. You have enabled the "LEAVE COMMENTS?" setting in the "Pull Request Alerts" area in your Repo Settings page inside the Coveralls app.

@jchick-bentley
Copy link
Collaborator

@jchick-bentley I was enjoying the imagery of joyriding the repository around. Here's the steps I took on my fork to get Coveralls working.

  1. Sign us up for Coveralls.

  2. Follow the steps in the GitHub actions README.

    1. You have invited the @coveralls user to your repository as a collaborator
    2. You have enabled the "LEAVE COMMENTS?" setting in the "Pull Request Alerts" area in your Repo Settings page inside the Coveralls app.

Ok - let's complete this PR w/o taking those extra steps. Seems like I have to grant access to all of iTwin rather than just our repo that's using it. I'll seek approval from someone who can speak for all of iTwin.

@jchick-bentley
Copy link
Collaborator

@jchick-bentley I was enjoying the imagery of joyriding the repository around. Here's the steps I took on my fork to get Coveralls working.

  1. Sign us up for Coveralls.

  2. Follow the steps in the GitHub actions README.

    1. You have invited the @coveralls user to your repository as a collaborator
    2. You have enabled the "LEAVE COMMENTS?" setting in the "Pull Request Alerts" area in your Repo Settings page inside the Coveralls app.

Ok - let's complete this PR w/o taking those extra steps. Seems like I have to grant access to all of iTwin rather than just our repo that's using it. I'll seek approval from someone who can speak for all of iTwin.

@akshay-madhukar @admccarthy1 can we please get one more reviewer before we merge?

@admccarthy1
Copy link
Contributor

sorry about the delay, I didn't see the email this morning because my internet is out (again) but looks good to me.

@jackson-at-bentley
Copy link
Contributor Author

What's the dependency policy? I don't know how to force reproducible builds with npm, I just npm install and hope the tests pass. Are the extensive changes to the lock file and package.json okay?

@jchick-bentley jchick-bentley merged commit 7681c36 into main Jul 12, 2022
@jchick-bentley jchick-bentley deleted the coverage branch July 12, 2022 21:13
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.

None yet

3 participants