refactor(developer): unify test action #12736
Conversation
mcdurdin
requested review from
jahorton,
srl295,
darcywong00,
SabineSIL and
markcsinclair
as code owners
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
github-actions
bot
added
developer/
common/
developer/compilers/
common/resources/
| @@ -304,4 +304,30 @@ _select_node_version_with_nvm() { | |||
|
|
|||
| check-markdown() { | |||
| node "$KEYMAN_ROOT/resources/tools/check-markdown" --root "$1" | |||
| } | |||
|
|
|||
| builder_do_typescript_tests() { | |||
There was a problem hiding this comment.
Good refactoring.
Some questions about maintenance:
- Could we add a short preamble to note what this does?
- From a maintenance view, the
builder_prefix would make me assume this function is defined inbuilder.inc.shwith all the otherbuilder_functions.
There was a problem hiding this comment.
- Could we add a short preamble to note what this does?
Yes, will do.
- From a maintenance view, the
builder_prefix would make me assume this function is defined inbuilder.inc.shwith all the otherbuilder_functions.
I didn't want to put this into the main builder script, as it is specific to Typescript (and perhaps specific even to Developer use of Typescript; not sure it would transplant to /web for example). Ideally, we may have a set of builder sub-scripts, e.g. builder.typescript.inc.sh, builder.meson.inc.sh, etc, which we can draw from, and I would like to consider that for 19.0 (perhaps low pri though). shellHelperFunctions.sh is a bit of a random grab bag of functions that need refactoring and renaming, and eventually it should go away.
There was a problem hiding this comment.
builder.typescript.inc.sh etc feat documented in #12744
| # note: `export TEST_SAVE_ARTIFACTS=1` to save a copy of artifacts to temp path | ||
| # note: `export TEST_SAVE_FIXTURES=1` to get a copy of cloud-based fixtures saved to online/ | ||
| # TODO: -skip-full | ||
| builder_run_action test builder_do_typescript_tests 70 |
There was a problem hiding this comment.
ok, and presumably we can crank the number up as tests improve..
There was a problem hiding this comment.
yes, you get a nice little warning that coverage is low. As tests are added, I have been bumping up the coverage number already. This is a little easier to read 😁
| THRESHOLD_PARAMS="--lines $C8_THRESHOLD --statements $C8_THRESHOLD --branches $C8_THRESHOLD --functions $C8_THRESHOLD" | ||
| fi | ||
|
|
||
| c8 --reporter=lcov --reporter=text --exclude-after-remap --check-coverage=false $THRESHOLD_PARAMS mocha ${MOCHA_FLAGS} "${builder_extra_params[@]}" |
There was a problem hiding this comment.
the extra params make me think that -- -f someTest or similar might be able to invoke a specific subtest. Is that the general intent?
There was a problem hiding this comment.
Yes, this was inconsistently implemented but that's exactly how it is supposed to be used.
I also wonder about a --no-eslint flag when doing repeated runs during unit test development, because it is slow.
|
Changes in this pull request will be available for download in Keyman version 18.0.149-alpha |
@keymanapp-test-bot skip
Follows: #12710