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

breaking(npm): bump dependencies #225

Merged
merged 8 commits into from
Mar 31, 2020
Merged

breaking(npm): bump dependencies #225

merged 8 commits into from
Mar 31, 2020

Conversation

erisu
Copy link
Member

@erisu erisu commented Mar 31, 2020

Motivation and Context

Bring all dependencies up to date (latest).

Description

Requires #223 to be merged first

Dev Dependencies

  • @cordova/eslint-config@^3.0.0
  • cordova-android@^8.1.0
  • cordova-ios@^5.1.1
  • eslint-plugin-es5@^1.5.0
  • grunt@^1.1.0
  • jasmine-core@^3.5.0
  • karma@^4.4.1
  • karma-chrome-launcher@^3.1.0
  • karma-coverage@^2.0.1
  • karma-jasmine@^3.1.1
  • puppeteer@^2.1.1

Dependencies

  • execa@^4.0.0
  • fs-extra@^9.0.0
  • globby@^11.0.0

Applied execa fix.

Testing

  • npm t

Checklist

  • I've run the tests to see all new and existing tests pass

- @cordova/eslint-config@^3.0.0
- cordova-android@^8.1.0
- cordova-ios@^5.1.1
- eslint-plugin-es5@^1.5.0
- grunt@^1.1.0
- jasmine-core@^3.5.0
- karma@^4.4.1
- karma-chrome-launcher@^3.1.0
- karma-coverage@^2.0.1
- karma-jasmine@^3.1.1
- puppeteer@^2.1.1
- execa@^4.0.0
- fs-extra@^9.0.0
- globby@^11.0.0
@erisu
Copy link
Member Author

erisu commented Mar 31, 2020

@raphinesse

Karma testing and reporting seems to be functioning properly but an UncaughtException error is now being reported and displayed in the terminal output. This appears with the bumped dep. Doesn't seem to be affecting or breaking the tests or reporting.

31 03 2020 08:32:37.974:ERROR [karma-server]: UncaughtException

logLevel=DEBUG

31 03 2020 17:50:34.930:ERROR [config]: Error in config file!
  Error: Cannot find module '/temp/cordova-js/—logLevel=DEBUG'
Require stack:
- /temp/cordova-js/node_modules/karma/lib/config.js
- /temp/cordova-js/node_modules/karma/lib/server.js
- /temp/cordova-js/node_modules/karma/lib/cli.js
- /temp/cordova-js/node_modules/karma/bin/karma
  at Function.Module._resolveFilename (internal/modules/cjs/loader.js:982:15)
  at Function.Module._load (internal/modules/cjs/loader.js:864:27)
  at Module.require (internal/modules/cjs/loader.js:1044:19)
  at require (internal/modules/cjs/helpers.js:77:18)
  at Object.parseConfig (/temp/cordova-js/node_modules/karma/lib/config.js:357:22)
  at new Server (/temp/cordova-js/node_modules/karma/lib/server.js:67:24)
  at Object.exports.run (/temp/cordova-js/node_modules/karma/lib/cli.js:295:7)
  at Object.<anonymous> (/temp/cordova-js/node_modules/karma/bin/karma:3:23)
  at Module._compile (internal/modules/cjs/loader.js:1158:30)
  at Object.Module._extensions..js (internal/modules/cjs/loader.js:1178:10)
  at Module.load (internal/modules/cjs/loader.js:1002:32)
  at Function.Module._load (internal/modules/cjs/loader.js:901:14)
  at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:74:12)
  at internal/main/run_main_module.js:18:47

npm ERR! Test failed.  See above for more details.

I am guessing something changes in their configurations but it doesn't nicely say what.

@raphinesse
Copy link
Contributor

@erisu I think you gave the wrong commandline option. npx karma start --log-level debug gives different output for me. Unfortunately still no details about the uncaught exception.

@raphinesse
Copy link
Contributor

@erisu The failure is caused by the major update of karma-coverage. Reverting the version change resolved the problem for me. Their list of breaking changes for v2 is quite long.

If you feel so inclined to fix this instead of downgrading, here's where we use the Istanbul API:

const contents = instrumenter.instrumentSync(f.contents, f.path);

Copy link
Contributor

@raphinesse raphinesse left a comment

Choose a reason for hiding this comment

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

Either revert version change of karma-coverage or fix our coverage setup to work with the new version please.

@erisu
Copy link
Member Author

erisu commented Mar 31, 2020

31 03 2020 11:06:11.607:ERROR [karma-server]: [TypeError: Cannot read property 'start' of undefined

@erisu
Copy link
Member Author

erisu commented Mar 31, 2020

Coverage is working with error. Configuration settings is identical to documentation.

@raphinesse
Copy link
Contributor

Three people fixing it at the same time in three different ways :D

3b337e8

@raphinesse
Copy link
Contributor

I basically did the same as @timbru31 but with a comment change and destructuring. Works for me to fix the Uncaught Exception. Why the commit pinning @erisu?

@erisu
Copy link
Member Author

erisu commented Mar 31, 2020

@raphinesse that is master at current commit id. In karma-coverage they updated their dependencies 6 days ago for the reporter.

-    "istanbul-lib-coverage": "^2.0.5",
-    "istanbul-lib-instrument": "^3.3.0",
-    "istanbul-lib-report": "^2.0.8",
-    "istanbul-lib-source-maps": "^3.0.6",
-    "istanbul-reports": "^2.2.4",
-    "minimatch": "^3.0.0"
+    "istanbul-lib-coverage": "^3.0.0",
+    "istanbul-lib-instrument": "^4.0.1",
+    "istanbul-lib-report": "^3.0.0",
+    "istanbul-lib-source-maps": "^4.0.0",
+    "istanbul-reports": "^3.0.0",
+    "minimatch": "^3.0.4"

In these changes, it fixed the error. I didn't want to leave it unpinned to always use lasted master as they might commit an accidental breaking change.

@raphinesse
Copy link
Contributor

@erisu But why do we need to use an unreleased version? It is fixed for latest released v2 by Tim's and my changes.

@erisu
Copy link
Member Author

erisu commented Mar 31, 2020

@raphinesse @timbru31 commit was pointing to master.

@timbru31
Copy link
Member

Indeed, the fix with createInstrumenter works with the v2.0.1, too

@codecov-io
Copy link

codecov-io commented Mar 31, 2020

Codecov Report

Merging #225 into master will decrease coverage by 0.85%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #225      +/-   ##
==========================================
- Coverage   85.08%   84.23%   -0.86%     
==========================================
  Files          14       14              
  Lines         570      539      -31     
==========================================
- Hits          485      454      -31     
  Misses         85       85              
Impacted Files Coverage Δ
src/common/builder.js 84.00% <0.00%> (-1.46%) ⬇️
src/common/pluginloader.js 87.75% <0.00%> (-1.14%) ⬇️
src/common/argscheck.js 93.10% <0.00%> (-0.84%) ⬇️
src/common/utils.js 73.52% <0.00%> (-0.76%) ⬇️
src/common/init.js 71.42% <0.00%> (-0.67%) ⬇️
src/cordova.js 58.44% <0.00%> (-0.54%) ⬇️
src/scripts/require.js 94.87% <0.00%> (-0.37%) ⬇️
src/common/modulemapper.js 96.29% <0.00%> (-0.26%) ⬇️
src/common/channel.js 96.59% <0.00%> (-0.12%) ⬇️
src/common/base64.js 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8fab0a...54302ba. Read the comment docs.

@erisu erisu merged commit 2444b1f into apache:master Mar 31, 2020
@erisu erisu deleted the update-deps branch March 31, 2020 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants