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

Include test coverage results in GitHub Action CI script #1148

Open
dondi opened this issue Nov 13, 2024 · 15 comments
Open

Include test coverage results in GitHub Action CI script #1148

dondi opened this issue Nov 13, 2024 · 15 comments

Comments

@dondi
Copy link
Owner

dondi commented Nov 13, 2024

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

@akaiap
Copy link
Collaborator

akaiap commented Nov 20, 2024

  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

@dondi
Copy link
Owner Author

dondi commented Nov 20, 2024

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

@akaiap
Copy link
Collaborator

akaiap commented Dec 4, 2024

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.

@dondi
Copy link
Owner Author

dondi commented Jan 15, 2025

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

@akaiap
Copy link
Collaborator

akaiap commented Jan 22, 2025

  • 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

@dondi
Copy link
Owner Author

dondi commented Jan 22, 2025

@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

@akaiap
Copy link
Collaborator

akaiap commented Jan 28, 2025

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

@dondi
Copy link
Owner Author

dondi commented Jan 29, 2025

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

@akaiap
Copy link
Collaborator

akaiap commented Feb 5, 2025

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

akaiap commented Feb 5, 2025

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

@dondi
Copy link
Owner Author

dondi commented Feb 5, 2025

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

@akaiap
Copy link
Collaborator

akaiap commented Feb 19, 2025

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.

@dondi
Copy link
Owner Author

dondi commented Feb 19, 2025

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

@akaiap
Copy link
Collaborator

akaiap commented Feb 26, 2025

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.

@dondi
Copy link
Owner Author

dondi commented Feb 26, 2025

@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

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

No branches or pull requests

3 participants