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

#1148 include test coverage in actions #1170

Open
wants to merge 15 commits into
base: beta
Choose a base branch
from

Conversation

akaiap
Copy link
Collaborator

@akaiap akaiap commented Feb 5, 2025

Capturing coveralls results to appear in the README.md as the badge.

@dondi dondi changed the base branch from main to beta February 12, 2025 05:53
Copy link
Owner

@dondi dondi left a comment

Choose a reason for hiding this comment

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

Generally looks good, but with some questions and a small potential revision before we proceed. Let’s address these then do another round

@@ -1,4 +1,5 @@
import "chai/register-should";
const chai = require("chai");
chai.should(); // Activates "should" style assertions
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a reason that this is calling require rather than import? The latter is the newer approach. Also, we should move chai.should() to after all of the import statements because the convention is to do all imports first before executing code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • Since Mocha and the Node environment were still configured for CommonJS modules, using require made the test file run correctly under the regular CommonJS system. Switching to import would require the project to be recognized as an ES module (e.g., by setting "type": "module" in package.json).

  • I will fix the import!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

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.

2 participants