GRNsight icon indicating copy to clipboard operation
GRNsight copied to clipboard

Include test coverage results in GitHub Action CI script

Open dondi opened this issue 1 year ago • 19 comments

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

dondi avatar Nov 13 '24 21:11 dondi

  1. 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
  1. Update README file
  • [![Coverage Status](https://coveralls.io/repos/github/<OWNER>/<REPO>/badge.svg?branch=master)](https://coveralls.io/github/<OWNER>/<REPO>?branch=master)
  • Test Test Test

akaiap avatar Nov 20 '24 19:11 akaiap

Instructions look good; learning about secrets systems comes with the territory so is a good aspect of these tickets

dondi avatar Nov 20 '24 20:11 dondi

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 ! Screenshot 2024-12-04 at 12 30 13 PM
  • 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.

akaiap avatar Dec 04 '24 20:12 akaiap

The next steps for @akaiap are listed above, plus she will verify the preferred location for the secret

dondi avatar Jan 15 '25 16:01 dondi

  • 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 -->
Image
  • 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 avatar Jan 22 '25 16:01 akaiap

@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 pwd command to get a full path for where you are

dondi avatar Jan 22 '25 16:01 dondi

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 coveralls command. As for right now -- this is the error you get when that command is run:
Image
  • 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 avatar Jan 28 '25 20:01 akaiap

@akaiap and @dondi will meet sometime next week to address the Coverall token issue

dondi avatar Jan 29 '25 16:01 dondi

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!)

  1. 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 test there are many build errors because of the node package problems. So for now, to test the coverage I am using the command continue-on-error: true # Linter failures won't stop the pipeline to 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 coveralls it works!

Image

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 coveralls on my local machine, I receive this error after the screenshot above: ``
Image

During our meeting, we can relink the github to coveralls.io!

akaiap avatar Feb 05 '25 07:02 akaiap

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

akaiap avatar Feb 05 '25 07:02 akaiap

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

dondi avatar Feb 05 '25 17:02 dondi

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.

akaiap avatar Feb 19 '25 08:02 akaiap

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

dondi avatar Feb 19 '25 16:02 dondi

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 using require. 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 avatar Feb 26 '25 11:02 akaiap

@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

dondi avatar Feb 26 '25 17:02 dondi

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.

akaiap avatar Mar 19 '25 11:03 akaiap

In #1189, I updated the minimum required Node version to ensure GRNsight continues running on the server.

akaiap avatar Mar 26 '25 08:03 akaiap

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

Image

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

dondi avatar Apr 09 '25 06:04 dondi

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

dondi avatar Apr 09 '25 07:04 dondi

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

dondi avatar Apr 23 '25 15:04 dondi

To trigger the linter manually, use npm run lint

dondi avatar Apr 23 '25 15:04 dondi

One remaining loose end for @akaiap is to investigate the behavior/link of the coverage badge

dondi avatar Apr 23 '25 15:04 dondi

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 pipeline in 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 :))))

akaiap avatar Apr 30 '25 06:04 akaiap

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.

akaiap avatar Apr 30 '25 07:04 akaiap

I looked at the readme.md on the beta branch and the coverage badge still goes to the 2017 release.

kdahlquist avatar Sep 03 '25 16:09 kdahlquist

Link appeared to work during meeting so will just mark as ready for next release

dondi avatar Sep 03 '25 17:09 dondi

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.

kdahlquist avatar Sep 10 '25 19:09 kdahlquist