Include test coverage results in GitHub Action CI script
In addition to running tests, we should start recapturing coverage results from the tests (formerly handled by Coveralls) and include this in the script as well. Per #1146, once we have coverage captured, it would be good to also include these results in the README badge
- I will modify the Github Actions workflow to generate coverage reports
- run:
npm test -- --coverage --coverageReporters=text-lcov | coveralls env: COVERALLS_REPO_TOKEN: ${{ secrets.COVERALLS_REPO_TOKEN }}Need to confirm coveralls account in repo secrets
- Update README file
-
[](https://coveralls.io/github/<OWNER>/<REPO>?branch=master) - Test Test Test
Instructions look good; learning about secrets systems comes with the territory so is a good aspect of these tickets
What I have done so far
- Updated the scripts in mocha
- Did some tested and updated mocha's version.
-coverage: Uses nyc with mocha to run tests and collect coverage data.
coveralls: Generates a coverage report in LCOV format and pipes it to coveralls.
- After removing Istanbul and installed I did some testing using
npm run coverage and npm run coveralls - and success !
- However, adding the coverage badge can get complicated due to GitHub "secrets." These secrets are admin tokens for Coveralls, and the safest approach is to avoid committing the token.
next steps to get the token: Sign in to Coveralls.io with GRNSight account. Enable repository if not already active. Navigate to the repository's settings on Coveralls. Copy the Repo Token provided. Add the Token to GitHub Repository Secrets:
Click on Settings > Secrets and variables > Actions. Click on New repository secret. Name the secret COVERALLS_REPO_TOKEN. Paste the token you copied from Coveralls. Save the secret.
This way this token will not be pushed to the repo.
The next steps for @akaiap are listed above, plus she will verify the preferred location for the secret
- While progressing to the next steps, I wanted to make sure I reproduce
npm run coverage and npm run coveralls - However, after
npm install, I was not able to npm run converalls or npm run coverage. - receiving this error -->
- Even after uninstalling Istanbul (and reinstalling) + reinstalling nyc I still receive the error. It looks like the local coverage script is still referencing istanbul cover _mocha somewhere, despite the package.json (but using NYC) not referencing Istanbul.
- Within the terminal above I used the command
npm run coverage --dry-run-- and received the same error. - Deleted package-json and node_modules -->
npm install - Reasons: Within my own local copy
@akaiap reported on the actions she took leading to the comment above; it looks like some setup questions need to be resolved:
- Double-check that the coverage branch being worked on has indeed been pushed to
origin - Double-check that the folder which VSCode is displaying is the same one as the folder on the command line
- One pre-step is to open a terminal in VSCode (which is guaranteed to be the same one as the files being viewed) to see if that terminal behaves any differently
- Use
pwdcommand to get a full path for where you are
I found the problem!
- I have multiple GRNsight folders on my machine. I was cloned into the wrong one (lol) and this is why when I tried to replicate the command
npm run coverage, it did not run and still referenced Istanbul instead of nyc. - The branch is there, so now to push this I need to follow the steps to run coveralls
npm run coverallscommand. As for right now -- this is the error you get when that command is run:
- While following the steps of attaining the coverage token, it still needs to be referenced within the yml file.
- Ready to go! All we need is the token, which we can review tomorrow! :)
@akaiap and @dondi will meet sometime next week to address the Coverall token issue
I met with @dondi today and successfully generated the token from Coveralls.io and placed it in GRNsights secrets. However, I ran into a couple of problems when building. (Very close!)
- When run the the build would fail for a couple of reasons (nitty gritty)
-
When running all the node versions, npm ci exited the build due to (too many) deprecated packages and corrupted/missing dependencies.
-
I updated 53/53 dependencies, but there are still more node packages that need to be deleted (they don't exist anymore) and manually updated.
-
When running
npm lint, npm ci, and npm testthere are many build errors because of the node package problems. So for now, to test the coverage I am using the commandcontinue-on-error: true # Linter failures won't stop the pipelineto continue through the build to test coveralls. - With this specific issue, many of the node modules need to updated, deleted, and even changed-- so this might have to be within this issue as well. -
After a lot of trial and error --> Solution for coveralls: (hopefully) We need to enable/relink coveralls.io to GRNsight within the website so it can recognize the token.
-
On positive note when I run
npm run coverallsit works!
Coveralls Error Summary:
- When run in github actions, I receive the error
🚨 ERROR: Couldn't find specified file: ./coverage/lcov.info - However, the file is there (when manually updated)
- Additionally when running
npm run coverallson my local machine, I receive this error after the screenshot above: ``
During our meeting, we can relink the github to coveralls.io!
Interesting development— After initially considering relinking GitHub to Coveralls.io as the solution, I decided to try one more thing first:
- On my local machine, I confirmed that the LCOV file (a test coverage report generated by NYC/Istanbul) existed and contained valid coverage data.
- Since the file was present, I attempted a manual upload, but encountered the notorious
Couldn't find a repository matching this job (422 error). - I then updated the GitHub Actions YAML file to properly specify the coverage file path.
- And it worked! Coveralls successfully located lcov.info, uploaded the coverage report, and GRNsight was successfully built!
Key Takeaways:
- npm lint is still set to continue-on-error, meaning there may still be outdated or incompatible packages that need further updates.
- The parsing error persists on my local machine, but GitHub Actions processes the coverage report without issues.
During our meeting we should still try to relink GRNsight with Coveralls.io
In order to get the badge, @dondi needs to look into reintegration of Coveralls with GitHub; @akaiap should look into documenting the overall version updates required, particularly NodeJS
I tested the build again after moving the chai.should() call below all the import statements. While the build still ran, it triggered warnings and linting errors.
- The issue is from an inconsistent module system, where ES Modules (import) are being processed before CommonJS (require). Moving chai.should() below the imports affects the execution order in a way that conflicts with the current module setup.
- Linting failures are due to ESLint 9 deprecating old configuration formats: ESLint now expects the "ignores" rule inside eslint.config.js instead of relying on the .eslintignore file. The "languageOptions" object no longer allows "ecmaFeatures", requiring an updated configuration. We can go down to V8, but may still encounter the same error.
Possible solutions:
CommonJS vs. ES Modules
Option 1: Convert the Project to Full ES Modules
- Add "type": "module" to package.json.
- Change all CommonJS require calls to import.
- Update .cjs files to .mjs if necessary.
- Ensure dependencies support ESM.
Option 2: Use a Hybrid Approach (Conditional Imports) (supporting both esm and commonjs)
(async () => { const chai = await import("chai"); chai.should(); })();
Fixing ESLint Issues
Option 1: Update eslint.config.js to Match ESLint 9 Standards
- Remove deprecated properties like "ecmaFeatures" and "environments".
- Move ignored files into ignores.
Option 2: Downgrade ESLint
- Downgrade back to ESLint 8:
Version Updates
Updates within NodeJs:
-
Updated Dependencies
| Package | Old Version | New Version |
|---|---|---|
| Express | 4.16.0 | 4.21.2 |
| Cytoscape | 2.7.14 | 3.31.0 |
| Sequelize | 5.21.6 | 6.37.5 |
| Moment.js | 2.24.0 | 2.30.1 |
| Webpack | 4.0.0 | 5.97.1 |
| Mocha | 2.5.3 | 11.1.0 |
| Coveralls | 2.13.1 | 3.1.1 |
Package-lock.json Changes Significant modifications were made to package-lock.json due to:
- New dependencies being installed and updated.
- Version bumps reflecting updates in the dependency tree.
- Security patches and performance enhancements.
- Pruning of unused dependencies.
- Replacements for deprecated packages, particularly those older than 5 years.
For the issues documented above, we’ve determined the following next steps:
- Issue 1 is documented in a PR comment
- For Issue 2, let’s look further into configuration changes due to the library update; the most likely cause is that the new version can no longer find our linter settings or the revisions to them might have resulted in missed or misinterpreted linter rules (.eslintignore, .eslintrc.yml)
- For Issue 3, the wiki page containing this information is https://github.com/dondi/GRNsight/wiki/Library-Requirements
Here's a summary of my findings and proposed solutions:
1. ESLint Configuration and Linting Errors
Migration to ESLint version 9 has caussed challenges due to changes in its config structure:
-
Configuration Format Changes: ESLint 9 has deprecated the traditional .eslintrc.* configuration files in favor of a new eslint.config.js format. This transition requires to adapt to the new structure. ESLint Migration Guide --> This recommends dynamic
-
Deprecated Properties: Properties such as ecmaFeatures and environments are no longer supported in ESLint 9. Attempting to use these deprecated properties results in linting errors. (explains the couple thousand errors)
-
Ignore Patterns: The previous .eslintignore file is now obsolete. ESLint 9 expects ignore patterns to be defined within the eslint.config.js file under the ignores property.
Proposed Solutions:
-
Update Configuration: Revise eslint.config.js to comply with ESLint 9 standards by removing deprecated properties and incorporating ignore patterns directly within the configuration file.
-
Node.js Version Compatibility: ESLint 9 requires at least Node.js v18.18.0. Migating to v9x
2. chai.should() Integration Issues
-
The errors come from the placement of chai.should() about import statements are because of module system inconsistencies:
-
Module System Conflict: GRNsight project utilizes both CommonJS (
require) and ES Modules (import). Chai has transitioned to an ES Module in its latest versions, leading to conflicts when usingrequire. GitHub Issue on Chai v5
Proposed Solutions:
- Dynamic Import in CommonJS: Modify CommonJS modules to dynamically import chai using import(). This approach allows us to load ES Modules within a CommonJS context.
(async () => {
const chai = await import('chai');
chai.should();
})();
an example of what the dynamic approach will look like-- will test to verify.
- Revert to an Earlier Version of chai: Alternatively, downgrading to a version of chai that supports CommonJS (e.g., version 4.2.0) can resolve the compatibility issue. Downgrading
3. Documentation Updates
I have begun drafting updates to the wiki, particularly focusing on the recent changes to thq library dependencies and configuration files. Once the linting issues are fully resolved, I will push these updates to the wiki.
Next Steps:
-
Finalize ESLint Configuration
-
Resolve chai Integration: test between implementing dynamic imports or reverting to a compatible version of chai to eliminate module system conflicts.
-
update Documentation: Complete and push the documentation the changes.
@akaiap will separate this ticket into at least a documentation issue and configuration/Chai issues, or possibly three distinct ones
After concluding research, go ahead update the PR as needed so we can go back to a review phase
Updates
ESLint Configuration & Linting Fixes:
- Updated eslint.config.js to worl with ESLint v9 standards.
- Linting errors significantly reduced
chai.should() Integration:
- Tested dynamic import approach for Chai in CommonJS. It successfully resolves the module conflict.
- Confirmed that changing to
"type": "module"in package json is a viable alternative if needed-- however... - All .js files would be treated as ES modules instead of CommonJS
- Major code changes required
- All calls would need to be replaced with import
- All module.exports = would need to be changed to export
- Files using CommonJS would need to be renamed with .cjs extension
- Mocha and testing setup might need reconfiguration
- Some older testing libraries might have compatibility issues
- Some dependencies might not be fully compatible with ESM
- might need to update or replace certain packages
This would be a big migration with thorough testing.
Documentation Updates:
- Updated Library Requirements wiki page with the latest package versions and installation instructions.
- Included notes on ESLint 9 migration, Chai v5 module conflicts, and the Istanbul → NYC transition.
- Made a separate issue for updates.
In #1189, I updated the minimum required Node version to ensure GRNsight continues running on the server.
I merged beta into this branch and switched to it on the server. Due to the package changes, npm install should be the next step, but when I attempted to do this, the install process hangs and results in a deadlocked process (see screenshot). Closing the command line window has been the only way to recover
Due to these incomplete install attempts, the beta server is currently non-operational. The production server is currently running and I hesitate to reboot the machine. Will need to consult with the group tomorrow on what next steps may be best
I have managed to get beta to at least reload for me, but with what is very much a bandaid-fix: I manually copied the node_modules folder from production into the beta folder. The web app serves up again and nominally appears to work, but this feels very fragile having it run off the copied node_modules from production
At least it appears to be back up and running but a more stable solution needs to be found here
PR has been merged into beta, so the rest of the team’s pushes can now start using the workflow. The latest beta will need to be merged into current working branches, and after that, pushes will trigger the GitHub actions
To trigger the linter manually, use npm run lint
One remaining loose end for @akaiap is to investigate the behavior/link of the coverage badge
Last week: Currently, so happy right now!! After months of getting the test coverage working, the badge is being updated, and coverage is passing!
Steps: After cleaning up the code to now take imports/export of grnState, the code was pushed and passed all the checks!
Next steps:
- While the checks pass -- I currently have
continue-on-error: true # Linter failures won't stop the pipelinein the yml package when Grnisight is built. - When I lint locally, I am receiving
1403 problems (1403 errors, 0 warnings) - Realized I was using two active and conflicting config files. (.eslint.config.js and eslintrc.yml)
- The next steps are to delete the old format (yml file) and reformat the config file to be able to use the
--fix command
ITS WORKING!
Steps taken
-
Fixed module system config: Properly configured ESLint to support ES Modules w
-
Resolved quote style issues: Fixed inconsistent quotes (single vs. double quotes)
-
Proper configuration: Set up appropriate globals and rules in the eslint.config.js file
Clean code base: the code now passes all linting checks, making it more maintainable and consistent :))))
Coverage Badge in README
The coverage badge in the README is currently broken because it’s still pointing to the master branch, which no longer exists. For now, we can update the badge to point to the beta branch where coverage is being reported.
-
/GRNsight?branch=beta)We can change to main once merged into main. - Coveralls does not recognize the main branch until coverage data is sent from it. Once CI runs on main and uploads coverage, it will start appearing in Coveralls.
I will create a PR that changes that updates the badge in the beta branch. Once main begins pushing coverage, we can update the badge again.
I looked at the readme.md on the beta branch and the coverage badge still goes to the 2017 release.
Link appeared to work during meeting so will just mark as ready for next release
I'm going to close this because it is finished. We can look at the closed issues when we are ready to write the release notes.