change(web): config, sourcemap cleanup + adds some c8 reporting 🧩 #8995
Conversation
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
common/models/templates/package.json
Outdated
| "build": "gosh ./build.sh", | ||
| "tsc": "tsc", | ||
| "mocha": "mocha", | ||
| "test": "mocha -r test/helpers.js" | ||
| "test": "mocha -r test/helpers.js", | ||
| "c8": "c8" |
There was a problem hiding this comment.
Can we make these use build.sh, and then we don't need tsc, mocha, or c8 scripts also:
"test": "gosh ./build.sh test"
There was a problem hiding this comment.
I didn't even notice the test bit.
That said, I like having tsc, mocha and the like so I can directly run those calls on occasion outside of build.sh scripts. I definitely use this from time to time with mocha (and karma for DOM-reliant Web components).
There was a problem hiding this comment.
You can of course do $KEYMAN_ROOT/node_modules/.bin/tsc etc (or even add your node_modules/.bin to your path)
| @@ -1,6 +1,6 @@ | |||
| import { assert } from 'chai'; | |||
|
|
|||
| import ManagedPromise from '../../build/obj/managedPromise.js'; | |||
| import { ManagedPromise } from '@keymanapp/web-utils'; | |||
There was a problem hiding this comment.
Hmm, is this self-referential? Is that a good thing?
There was a problem hiding this comment.
src/test/...: we're importing the package for its unit test. Importing through the primary index helps c8 to properly assess coverage for all files, even if we manually exclude some in the config. Directly referencing the raw built modules made a difference for c8 for some reason.
Tackles a few small points and one maybe-medium point:
I've been noticing some noise and inconsistencies with the reported path locations for original sourcefiles in the output sourcemaps. For example, sometimes subdirectories of the main
src/would get duplicated in said paths. It turns out that this was due to use of thesourceRootconfig option in some of the TS config files; removing it prevents that duplication.When viewing KMW's source in developer mode, the layout of the sources didn't quite match the repository layout. We can workaround it by setting the
esbuildsourceRoot to a higher-level directory for the repo or similar. This is now fixed:At one point, I thought about having it set with this repository's URL as the effective root, so that the paths would reflect their in-repo locations. Once I realized that it'd be far to tricky to point to a specific tag or similar - instead, always pointing to the
masterversion, I reconsidered.I'd be perfectly happy to change the name on that root if desired.
c8within thekmcin-repo project, I decided to try it out with Web and its submodules - and for most headless cases, I was able to get it working! It only generates reports for us; it won't cause any failing builds at this time.common/web/keyboard-processor:common/models/wordbreakers:common/models/templates:common/web/lm-worker:web/src/engine/package-cache:I was pretty pleasantly surprised to see the numbers this high on these components!
Unfortunately, I ran into a blocker for doing this when headless integration tests involving both the main thread and the worker were involved - thus,
common/web/input-processorandcommon/predictive-textdo not generate reports at this time. I know how to bypass the issue and get reports manually upon request, though.I submitted an issue at bcoe/c8#477 for the problem I ran into; my hacky 'bypass' is mentioned there too. As it requires editing one of the c8 package's scripts directly, I don't consider the 'bypass' viable as a workaround at this time.
Using the bypass:
common/web/input-processor:This makes sense; the automated tests here mostly exist to ensure integration with the worker isn't broken.
common/predictive-text:@eddieantonio was quite thorough with establishing unit tests when we were collaborating on the predictive-text feature's development, and it shows - both here and in all the other component packages listed above. (Many thanks!)
While I did experiment a bit with attempting cross-project coverage... that got messy very quickly, so I backed out of it for now. I can tell that
c8does have some sort of internal report-merging mechanism, and it would be nice to leverage that, but it's not currently worth the time investment required.Additionally, this had the fortunate benefit of helping me to discover and resolve some previously-uncaught sourcemap issues.
lm-workerproject no longer needs the 'sourcemap remapper' tool I previously wrote. The path-mapping is now handled far more cleanly than before.@keymanapp-test-bot skip