Generate code coverage reports on PRs

[CyberCitizen01]

Closes #203

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: CyberCitizen01 / name: Mohammad Rafivulla (c25652c, ee28b68, d68fc19, 4136628, 05cf4b2, bf712c1, 045a47d, 0d81a41, 5b1655f, 222a3dc, b164608, 36421b7, a3e86ed, 5bbf789, e1d6117, e40cfce, 9db4525)
• ✅ login: JamieSlome / name: Jamie Slome (a37a3f6, cc21b82, 8182d65, 5d449e1, 06964ce, bf12488, b5f00a1)

[CyberCitizen01]

Hi
I've just noticed that the latest workflow run in this repository has failed. I've a hunch that it has something to do with permissions seeing the 403. Can I get a screenshot of the Settings -> Action -> General -> Workflow permissions page or maybe you can confirm if everything seems fine (maybe the action romeovs/lcov-reporter-action needs to be added in the Allow specified actions and reusable workflows list?).
Thanks!

[JamieSlome]

@TheJuanAndOnly99 @maoo - I defer to you here. It appears the Action is not running as expected. Any ideas?

[TheJuanAndOnly99]

Hi @JamieSlome apologies for the delay. We're just getting back from some time off. We'll take a look as soon as we can and get back to you!

[JamieSlome]

@TheJuanAndOnly99 - no worries, thanks for the update, and hope you had a good break! 👍

[JamieSlome]

@CyberCitizen01 - in the meantime, could you take a look at the merge conflicts?

[TheJuanAndOnly99]

Hi @CyberCitizen01 @JamieSlome The settings seem fine. Action Permissions are set to allow all and Workflow Permissions are set to read and write permissions. I may be wrong but it seems like you have configured it with a file that does not exist. Is that possible?

[CyberCitizen01]

@JamieSlome - I'll look into the merge conflicts right away.
@TheJuanAndOnly99 - Thanks for the update and your suggestion. I'll try to investigate this further and cross check if I've missed anything, although it seems to work for a dummy PR (CyberCitizen01#1) in my fork. So, it seems that GH possibly doesn't permit workflows in PRs to run with write permissions maybe?

[JamieSlome]

@CyberCitizen01 - thanks 👍

[JamieSlome]

@CyberCitizen01 - any updates here?

[CyberCitizen01]

Hi @JamieSlome apologies for the delay. I was caught up in my uni exams. I'll take a look as soon as possible and get back to you.

[JamieSlome]

@CyberCitizen01 - not a problem at all! I hope the exams went well 👍

[CyberCitizen01]

Hello there, sorry for being late.

I tried finding an appropriate GH action for "Assess coverage reports on new code only" #203 (comment) and at best I was able to discover this thread istanbuljs/nyc#377. To implement this, I think I'll be required to create a script, maybe something similar to the one in scripts folder. Also I was not able to find a GH action that does this, and I'm still trying to find one (maybe I can get some help in finding one?).

I would like to have some advice on how to move forward.

Thanks!

[coopernetes]

@CyberCitizen01 did some digging and I have a patch that should satisfy that requirement. Can you create a new file in your git repo (ie. "coverage.patch") using the contents below and use git apply coverage.patch to apply those changes to your branch and push?

To summarize the changes below:

• Converts your use of a static .nycrc.json config file to a scripted approach using nyc.config.js. This allows us to execute some arbitrary JavaScript code as part of setting the configuration in nyc when its invoked. See docs for details
• In nyc.config.js, we execute the command git diff --name-only to compare the list of changed files between the PR and the base ref (origin/main branch in most cases). The list of changed files is passed into the nyc's include config. See linked comment on actions/checkout describing this command. From that same thread, we also needed to add the fetch-depth: 0 config to the actions/checkout step in the workflow. By default, the checkout action only clones & checks out the latest commit for a branch and does not have the history in origin/main. Details in that issue as well.
• We only parse the list of changed files when a PR is detected. This is done by seeing if GitHub's default environment variable is set for GITHUB_BASE_REF. This only gets set when the action is triggered by a pull request. See GitHub docs. When you run coverage on your development environment, all files touched will be displayed in the coverage output:

Running the new coverage script locally

$ npm run test-coverage-ci

> @finos/[email protected] test-coverage-ci
> nyc --reporter=lcovonly --reporter=text --check-coverage npm run test

nyc config:  { branches: 80, lines: 80, functions: 80, statements: 80 }
...
ERROR: Coverage for lines (66.26%) does not meet global threshold (80%)
ERROR: Coverage for functions (55.88%) does not meet global threshold (80%)
ERROR: Coverage for branches (38%) does not meet global threshold (80%)
ERROR: Coverage for statements (64.3%) does not meet global threshold (80%)
----------------------------------|---------|----------|---------|---------|----------------------------------------------
File                              | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                            
----------------------------------|---------|----------|---------|---------|----------------------------------------------
All files                         |    64.3 |       38 |   55.88 |   66.26 |                                              
 packages/git-proxy-notify-hello  |    12.5 |      100 |       0 |    12.5 |                                              
  index.js                        |    12.5 |      100 |       0 |    12.5 | 2-11                                         
 src                              |   57.14 |       50 |   44.44 |   57.14 |                                              
  plugin.js                       |   57.14 |       50 |   44.44 |   57.14 | 35,38-48,74-121,130                          

Simulating the workflow under a PR

$ GITHUB_BASE_REF=main GITHUB_SHA=$(git rev-parse HEAD) npm run test-coverage-ci

> @finos/[email protected] test-coverage-ci
> nyc --reporter=lcovonly --reporter=text --check-coverage npm run test

Generating coverage report for changed files...
nyc config:  {
  branches: 80,
  lines: 80,
  functions: 80,
  statements: 80,
  include: [
    '.github/workflows/nodejs.yml',
    'README.md',
    'nyc.config.js',
    'package-lock.json',
    'package.json',
    'test/1.test.js',
    'test/addRepoTest.test.js',
    'test/testCheckRepoInAuthList.test.js',
    'test/testLogin.test.js',
    'test/testUserCreation.test.js',
    ''
  ]
}
...
----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------|---------|----------|---------|---------|-------------------
All files |       0 |        0 |       0 |       0 |                   
----------|---------|----------|---------|---------|-------------------

Here are working examples:

coopernetes#2
https://github.com/coopernetes/git-proxy/actions/runs/6598811047/job/17927159890
https://github.com/coopernetes/git-proxy/compare/62dad6808ba799e925ac654699eff9fa467fdece..1fc80f340fc2d22843e437d3436b069e2262c018

With a "synthetic commit" by changing a file with tests:
https://github.com/coopernetes/git-proxy/actions/runs/6598958219/job/17927468504?pr=2

FYI @JamieSlome

diff --git a/.github/workflows/nodejs.yml b/.github/workflows/nodejs.yml
index 84e8ee0..6cb4630 100644
--- a/.github/workflows/nodejs.yml
+++ b/.github/workflows/nodejs.yml
@@ -24,6 +24,8 @@ jobs:
 
     steps:
     - uses: actions/checkout@8ade135a41bc03ea155e62e844d188df1ea18608 # v4
+      with:
+        fetch-depth: 0
 
     - name: Use Node.js ${{ matrix.node-version }}
       uses: actions/setup-node@v3
diff --git a/.nycrc.json b/.nycrc.json
deleted file mode 100644
index a61301d..0000000
--- a/.nycrc.json
+++ /dev/null
@@ -1,6 +0,0 @@
-{
-  "branches": 80,
-  "lines": 80,
-  "functions": 80,
-  "statements": 80
-}
diff --git a/README.md b/README.md
index 60a5e4c..6ac7c0d 100644
--- a/README.md
+++ b/README.md
@@ -290,3 +290,4 @@ This project is distributed under the Apache-2.0 license. See <a href="./LICENSE
 
 If you have a query or require support with this project, [raise an issue](https://github.com/finos/git-proxy/issues). Otherwise, reach out to [[email protected]](mailto:[email protected]).
 
+
diff --git a/nyc.config.js b/nyc.config.js
new file mode 100644
index 0000000..2971607
--- /dev/null
+++ b/nyc.config.js
@@ -0,0 +1,41 @@
+/* eslint-disable max-len */
+'use strict';
+
+const { execFileSync } = require('child_process');
+
+let opts = {
+  branches: 80,
+  lines: 80,
+  functions: 80,
+  statements: 80,
+};
+
+// Only generate coverage report for changed files in PR
+// see: https://github.com/actions/checkout/issues/438#issuecomment-1446882066
+//      https://docs.github.com/en/actions/learn-github-actions/variables#default-environment-variables
+if (process.env.GITHUB_BASE_REF !== undefined) {
+  console.log('Generating coverage report for changed files...');
+  try {
+    const baseRef = execFileSync('git', [
+      'rev-parse',
+      `origin/${process.env.GITHUB_BASE_REF}`,
+    ])
+      .toString()
+      .replace('\n', '');
+    const headRef = process.env.GITHUB_SHA;
+    const stdout = execFileSync('git', [
+      'diff',
+      '--name-only',
+      `${baseRef}..${headRef}`,
+    ]).toString();
+    opts = {
+      ...opts,
+      include: stdout.split('\n'),
+    };
+  } catch (error) {
+    console.log('Error: ', error);
+  }
+}
+
+console.log('nyc config: ', opts);
+module.exports = opts;
diff --git a/package.json b/package.json
index 1bcaa42..82e1a12 100644
--- a/package.json
+++ b/package.json
@@ -13,7 +13,7 @@
     "server-test": "mocha --exit",
     "test": "mocha --exit",
     "test-coverage": "nyc npm run test",
-    "test-coverage-ci": "nyc --reporter=lcovonly --reporter=text-summary --check-coverage npm run test",
+    "test-coverage-ci": "nyc --reporter=lcovonly --reporter=text --check-coverage npm run test",
     "prepare": "node ./scripts/prepare.js",
     "lint": "eslint --fix . --ext .js,.jsx"
   },

[JamieSlome]

@CyberCitizen01 - thank you for the follow-up! 🎉

Happy to merge if you are @coopernetes...

[CyberCitizen01]

@JamieSlome @coopernetes

[coopernetes]

LGTM 👍

[CyberCitizen01]

Hello @JamieSlome

I wanted to ask and clarify, whether anything from my side is being expected. As it will be lovely if the PR gets merged soon. However I understand if maybe this PR is being regarded as a low priority for now. In that case please let me know if I can do anything to help around here.

Thank you and happy holidays 🎉

[coopernetes]

@CyberCitizen01 can you refresh the package-lock.json file by running npm install on your branch and committing the changes? I think that should be the last thing remaining and should fix the outstanding CI failure on the CVE scanning job. Then we should be able to merge.

[JamieSlome]

@coopernetes @CyberCitizen01 - I've sorted out our failing CI as it was affecting multiple PRs. Are you able to just resolve the outstanding conflict on the package-lock.json?

Once that's sorted we'll merge this PR 👍

[JamieSlome]

@CyberCitizen01 - just pinging on the above, are we able to get the merge conflict resolved? Very keen to get your fab contributions merged! 👍

[CyberCitizen01]

@JamieSlome - Done 🎉

[coopernetes]

@JamieSlome @CyberCitizen01 please have a look at the suggested change. We don't want to drag this out too much longer but I am concerned with the deprecation notice being generated.

[CyberCitizen01]

@JamieSlome @coopernetes - I've verified that the run of the updated workflow has the expected outcome.

CyberCitizen01#3 (comment)

I also went ahead and resolved our conversation (#213 (comment)).

Please verify and let me know if I can do anything to help.

Lets merge this PR 🦾!

[coopernetes]

Seems like the new LCOV report action fails when coverage data is missing. Ugh.

@CyberCitizen01 can you add if: success() || failure()¹ to that specific step? We at least have text-based coverage in the npm test step running now. We can tackle the action failure in a subsequent PR so that we at least start testing with coverage. Taking a cue from other FINOS repos, this isn't out of the ordinary.

@maoo wondering if we should be leveraging codecov instead for generating coverage reports outside of GitHub? It looks available for all FINOS repos² as far as I can tell.

Footnotes

1. https://stackoverflow.com/a/58859404/7102108 ↩

2. https://app.codecov.io/gh/finos ↩

[CyberCitizen01]

@coopernetes - adding if: success() || failure() to a failing step will not skip it. Instead I've just added a step to tackle the situation of an empty lcov.info file.

Rationale for forced push: I thought, if there are no tests for created/modified files lcov.info file is not generated altogether, however actually the file does get generated but it is empty.

[coopernetes]

LGTM - thank you @CyberCitizen01 for the contribution and sticking with it after many iterations!

[JamieSlome]

Nice job, @CyberCitizen01! Hopefully the first of many contributions, you're a star! ⭐

I have left a follow-up comment on #203 with regards to a failing PR which includes a bug fix and new test cases. If we could get some eyes on that, it would be appreciated.