[CS2] Fix async tests #4680
Merged
[CS2] Fix async tests #4680
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…t’ wrapper in the REPL to use a Promise instead of the ‘await’ keyword; add tests for REPL ‘await’ wrapper, including test to skip async tests if the runtime doesn’t support them
…` function is), we add it to an array of async test functions and wait for them all to resolve before finishing the test run, so that if any async tests fail we see those failures in the output
connec
reviewed
Sep 1, 2017
Cakefile
Outdated
| failures.push {filename, error} | ||
| return !failures.length | ||
|
|
||
| Promise.all asyncTests |
Collaborator
There was a problem hiding this comment.
I'd suggest trying to get a 'final' pass/fail from this method if possible, e.g. something like:
Promise.all(asyncTests).then ->
Promise.reject() if failures.lengthThen, when handling the results, we can just do:
runTests CoffeeScript
.catch -> process.exit 1
connec
reviewed
Sep 1, 2017
Cakefile
Outdated
| catch err | ||
| onFail description, fn, err | ||
|
|
||
| global.supportsAsync = if global.testingBrowser |
Collaborator
There was a problem hiding this comment.
I think I made this comment in another PR - but could we not just have the try feature-test only? It seems neater to me to have just the one:
global.supportsAsync = try
new Function 'async () => {}'
yes
catch
no
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Not sure when this started happening, but our current test runs complete before waiting for the async tests to resolve, either passing or failing, so if an async test were to start failing we would never know. To remedy this, I check if a test function is an
instanceof Promise, as all async functions are, and if it is, I save it to anasyncTestsarray. After the synchronous tests are all run, I doPromise.all asyncTeststo wait for all the async tests to resolve. The consumers of therunTestsfunction now expect aPromiseto be returned; for non-async runtimes, I return an immediately-resolvingPromisethat resolves to whether or not all the tests passed. (I wanted to preserve the ability for the tests to run in Node 6 and non-async-capable runtimes, since it isn’t so much extra effort at least for now. Node 6 supports promises.)To test this, break one of the tests (async and/or non-async) and make sure the failure appears when you run
cake test. Do the same when running in Node 6.(This branches off of #4679, so that it’s possible to run
cake testin Node 6. The diff should shrink once that PR is merged in.)