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

fix(refactor): remove stale check for createDynamicModule #451

Merged
merged 1 commit into from
Jan 4, 2024

Conversation

mcknasty
Copy link
Contributor

@mcknasty mcknasty commented Feb 2, 2023

Pull Request 451

    Increase Code Quality By Increasing Code Coverage: lib/is-cjs-esm-bridge.js

    refactor: Changed logic in is-cjs-ems-bridge file for lines of code that
    were not reached.

The purpose of this pull request is to increase coverage on the is-cjs-esm-bridge.js file. Below is a table of the results I was able to obtain from refactoring. One Node version 10.24.1, I was not able to get 100% coverage but since that version is in the process of being deprecated, and all other versions of node were able to obtain 100% coverage, I would like this pull request to be considered.

Following the Contributions Recommendations here.

  1. Make sure you have installed the latest version of Node.js
  2. Fork this project on GitHub and clone your fork locally
  3. Create local branches to work within. These should also be created directly from the main branch. Local Fork here.
  4. Make your changes.
  5. Run tests to make sure all is okay (everything should pass except the snapshot).
    1. A complete log of initial test results.
    2. As instructed, ignore snapshot failures. 2 failing
    3. 93 passing in 48 seconds
  6. Now update the snapshot.
    1. A complete log of snapshot test results.
    2. 95 passing in 50 seconds
  7. If all is passing, commit your changes. The log of the commit can be found here.
  8. As a best practice, once you have committed your changes, it is a good idea to use git rebase (not git merge) to synchronize your work with the main repository.
  9. Run tests again to make sure all is okay.
    1. A complete log of the final test results.
    2. 95 passing in 48 seconds
  10. Push
  11. Open the pull request, see details in the template.
  12. Make any necessary changes after review.

File Code Coverage Matrix Report

Test Type File Statement Branch Function Lines PASS
Node 10.24.1* lib/is-cjs-esm-bridge.js
Node 12.22.12 lib/is-cjs-esm-bridge.js 100% 100% 100% 100% ✔️
Node 14.21.3 lib/is-cjs-esm-bridge.js 100% 100% 100% 100% ✔️
Node 16.20.1 lib/is-cjs-esm-bridge.js 100% 100% 100% 100% ✔️
Node 18.17.0 lib/is-cjs-esm-bridge.js 100% 100% 100% 100% ✔️

*Node version 10.x is no longer supported

Unit Tests Results

Test Type PASS Tests Passed Tests Failed Time
Initial Test 93 passing 2 failing 48 seconds
Snapshot Test ✔️ 95 passing 0 failures 50 seconds
Final Test ✔️ 95 passing 0 failures 48 seconds

Node Version Testing Matrix

Node Version PASS Tests Passed Tests Failed Time
10.24.1*
12.22.12 ✔️ 95 passing 0 failures 1 minutes
14.21.3 ✔️ 95 passing 0 failures 1 minutes
16.20.1 ✔️ 95 passing 0 failures 1 minutes
18.17.0 ✔️ 95 passing 0 failures 1 minutes

*Node version 10.x is no longer supported

Referencing Issue #448

[Updated 2023-02-20 1:29PM EST - Making updates requested in conversation, updating branch and PR docs]
[Updated 2023-07-28 11:20PM EST - Rebasing Pull Request]

lib/is-cjs-esm-bridge.js Outdated Show resolved Hide resolved
@mcknasty mcknasty marked this pull request as ready for review February 2, 2023 19:28
mcknasty added a commit to mcknasty/c8 that referenced this pull request Feb 20, 2023
…dge.js (bcoe#451)

refactor: Changed logic in is-cjs-ems-bridge file for lines of code that
were not reached.
@mcknasty mcknasty force-pushed the is-cjs-ems-bridge-code-coverage branch from 1f359be to b443cdc Compare February 20, 2023 18:28
@mcknasty mcknasty marked this pull request as draft February 20, 2023 18:36
@mcknasty mcknasty marked this pull request as ready for review February 20, 2023 18:36
mcknasty added a commit to mcknasty/c8 that referenced this pull request Jul 29, 2023
refactor: Changed logic in is-cjs-ems-bridge file for lines of code that
were not reached.
@mcknasty mcknasty force-pushed the is-cjs-ems-bridge-code-coverage branch from b443cdc to 4c1e0e5 Compare July 29, 2023 02:20
mcknasty added a commit to mcknasty/c8 that referenced this pull request Jul 29, 2023
…dge-code-coverage.js (bcoe#451)

refactor: Changed logic in is-cjs-ems-bridge file for lines of code that were not reached.
@mcknasty mcknasty force-pushed the is-cjs-ems-bridge-code-coverage branch from 4c1e0e5 to d9bb553 Compare July 29, 2023 03:06
@bcoe
Copy link
Owner

bcoe commented Jan 3, 2024

Node 14 is now the minimum. Your comment suggests we don't need the CJS bridge any longer?

…dge-code-coverage.js (bcoe#451)

refactor: removed is-cjs-ems-bridge file and references
@mcknasty mcknasty force-pushed the is-cjs-ems-bridge-code-coverage branch from d9bb553 to 03cc9fc Compare January 3, 2024 21:56
@mcknasty
Copy link
Contributor Author

mcknasty commented Jan 3, 2024

Yes, I removed the cjs-esm-bridge file and it's references, and the unit tests all passed. Please review when you have a moment.

@bcoe bcoe changed the title Increase Code Quality By Increasing Code Coverage: lib/is-cjs-esm-bridge.js fix(refactor): remove stale check for createDynamicModule Jan 4, 2024
@bcoe bcoe merged commit 5e18365 into bcoe:main Jan 4, 2024
6 checks passed
mcknasty added a commit to mcknasty/c8 that referenced this pull request Jul 17, 2024
…dge-code-coverage.js (bcoe#451)

refactor: removed is-cjs-ems-bridge file and references
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