-
Notifications
You must be signed in to change notification settings - Fork 14
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 more SourceMaps issues #34
Fix more SourceMaps issues #34
Conversation
Current coverage is 75.44% (diff: 83.01%)@@ dev-1.0.0 #34 diff @@
===========================================
Files 22 22
Lines 779 794 +15
Methods 0 0
Messages 0 0
Branches 0 0
===========================================
+ Hits 589 599 +10
- Misses 190 195 +5
Partials 0 0
|
filepath.startsWith('../web.browser/') || filepath.startsWith(abspath.clientSide)); | ||
Log.info('Add source map for file', sourceMapPath); | ||
sourceMap.registerMap(filepath, fileContent); | ||
Log.timeEnd('END registerSourceMap', filepath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string passed to console.time
and console.timeEnd
needs to be the same. Otherwise the CI looks promising !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a pity, since it improved (my) experience reading the outputs (because it groups all the info about SourceMaps betweetn START/END). Does it have something to do with the CI? Log.time
and Log.timeEnd
only output to the console in server/conf/log.js
. Nevertheless, I'm changing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Log.time (string1)
starts a countdown and prints nothing whereas Log.timeEnd(string1)
stop the countdown and prints string1 + the corresponding time lapse
…on and pkg quantity)
Change requested done 😉 |
Feel free to push that commit |
I did a rebase this morning right after your comment, that's why it appears as a unique commit, but the change you requested is pushed. Check at |
I was redoing tests by myself before seeing that you attached logs to your pull request. Great work @cgalvarez . It looks perfect so far ! 🍸 |
Description
PR which fixes some more issues found during testing:
package.json
of app NPM dependency).meteor test-packages
that result from combining location (executing from inside/outside a package folder) and package quantity (passing the package location or ID). All the possibilites are documented insideSourceMap.initialSetup()
. 🤘meteor-coverage-app-exemple
too.somepkg
inmeteor-coverage-app-exemple
).meteor-coverage
, so I didn't test the two specific patterns of my package).meteor-coverage-app-exemple
to cover these cases without the need to test with my own package.map.sourcesContent
(warned through console when verbosity is enabled). Examples:meteor://💻app/imports/ui/components/template.lists-show.js
meteor://💻app/imports/ui/components/template.todos-item.js
meteor://💻app/imports/ui/pages/template.app-not-found.js
meteor://💻app/imports/ui/pages/template.lists-show-page.js
Motivation and Context
Solves more issues with SourceMaps sources.
How Has This Been Tested?
Tested with
meteor-coverage
andmeteor-coverage-app-exemple
.Testing environment:
Tests run and related ouputs (attached):
meteor-coverage$ meteor npm run test >
output.txtmeteor-coverage-app-exemple$ meteor npm run coverage-app-unit >
output.txtmeteor-coverage-app-exemple$ meteor npm run coverage-app-full >
output.txtmeteor-coverage-app-exemple$ meteor npm run coverage-packages-mocha >
output.txtTypes of changes