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

support promise return in ospec #1928

Merged
merged 4 commits into from Aug 31, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/change-log.md
Expand Up @@ -23,6 +23,11 @@

- API: Introduction of `m.redraw.sync()` ([#1592](https://github.com/MithrilJS/mithril.js/pull/1592))

#### Ospec improvements:

- Added support for async functions and promises in tests - ([#1928](https://github.com/MithrilJS/mithril.js/pull/1928))
- Error handling for async tests with `done` callbacks supports error as first argument

#### Bug fixes

- API: `m.route.set()` causes all mount points to be redrawn ([#1592](https://github.com/MithrilJS/mithril.js/pull/1592))
Expand Down
33 changes: 32 additions & 1 deletion ospec/README.md
Expand Up @@ -148,6 +148,22 @@ o("setTimeout calls callback", function(done) {
})
```

Alternativly you can return a promise or even use an async function in tests:

```javascript
o("promise test", function() {
return new Promise(function(resolve) {
setTimeout(resolve, 10)
})
})
```

```javascript
o("promise test", async function() {
await someOtherAsyncFunction()
})
```

By default, asynchronous tests time out after 20ms. This can be changed on a per-test basis using the `timeout` argument:

```javascript
Expand All @@ -158,7 +174,22 @@ o("setTimeout calls callback", function(done, timeout) {
})
```

Note that the `timeout` function call must be the first statement in its test.
Note that the `timeout` function call must be the first statement in its test. This currently does not work for promise tests. You can combine both methods to do this:

```javascript
o("promise test", function(done, timeout) {
timeout(1000)
someOtherAsyncFunctionThatTakes900ms().then(done)
})
```

```javascript
o("promise test", async function(done, timeout) {
timeout(1000)
await someOtherAsyncFunctionThatTakes900ms()
done()
})
```

Asynchronous tests generate an assertion that succeeds upon calling `done` or fails on timeout with the error message `async test timed out`.

Expand Down
60 changes: 38 additions & 22 deletions ospec/ospec.js
Expand Up @@ -84,40 +84,56 @@ module.exports = new function init(name) {
if (cursor === fns.length) return

var fn = fns[cursor++]
var timeout = 0, delay = 200, s = new Date
var isDone = false
Copy link
Member

Choose a reason for hiding this comment

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

Danger prevents the test suite from running, but I wonder how much time it takes to create these closures unconditionally?

if (fn.length > 0) { could become

var res; if (fn.length > 0 || (res = fn(), res != null && typeof res.then === 'function;)) {

And in that if block, assign done to a variable and, either call fn(done) or res.then(done)...

If it doesn't make sense don't worry, I'll have to read this with a fresh mind :-)


function done(err) {
if (err) {
if (err.message) record(err.message, err)
else record(err)
subjects.pop()
next()
}
if (timeout !== undefined) {
timeout = clearTimeout(timeout)
if (delay !== Infinity) record(null)
if (!isDone) next()
else throw new Error("`" + arg + "()` should only be called once")
isDone = true
}
else console.log("# elapsed: " + Math.round(new Date - s) + "ms, expected under " + delay + "ms")
}

function startTimer() {
timeout = setTimeout(function() {
timeout = undefined
record("async test timed out")
next()
}, Math.min(delay, 2147483647))
}

if (fn.length > 0) {
var timeout = 0, delay = 200, s = new Date
var isDone = false
var body = fn.toString()
var arg = (body.match(/\(([\w$]+)/) || body.match(/([\w$]+)\s*=>/) || []).pop()
if (body.indexOf(arg) === body.lastIndexOf(arg)) throw new Error("`" + arg + "()` should be called at least once")
try {
fn(function done() {
if (timeout !== undefined) {
timeout = clearTimeout(timeout)
if (delay !== Infinity) record(null)
if (!isDone) next()
else throw new Error("`" + arg + "()` should only be called once")
isDone = true
}
else console.log("# elapsed: " + Math.round(new Date - s) + "ms, expected under " + delay + "ms")
}, function(t) {delay = t})
fn(done, function(t) {delay = t})
}
catch (e) {
record(e.message, e)
subjects.pop()
next()
done(e)
}
if (timeout === 0) {
timeout = setTimeout(function() {
timeout = undefined
record("async test timed out")
next()
}, Math.min(delay, 2147483647))
startTimer()
}
}
else {
fn()
nextTickish(next)
Copy link
Member

Choose a reason for hiding this comment

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

IIRC there was a good reason to use nextTickish() here.

Copy link
Member Author

Choose a reason for hiding this comment

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

readd

var p = fn()
if (p && p.then) {
startTimer()
p.then(function() { done() }, done)
} else {
nextTickish(next)
}
}
}
}
Expand Down
49 changes: 48 additions & 1 deletion ospec/tests/test-ospec.js
Expand Up @@ -107,7 +107,7 @@ o.spec("ospec", function() {
o(output).deepEquals({tag: "div", children: children})
})
})
o.spec("async", function() {
o.spec("async callback", function() {
var a = 0, b = 0

o.before(function(done) {
Expand Down Expand Up @@ -148,4 +148,51 @@ o.spec("ospec", function() {
})
})
})

o.spec("async promise", function() {
var a = 0, b = 0

function wrapPromise(fn) {
return new Promise((resolve, reject) => {
callAsync(() => {
try {
fn()
resolve()
} catch(e) {
reject(e)
}
})
})
}

o.before(function() {
return wrapPromise(() => {
a = 1
})
})

o.after(function() {
return wrapPromise(function() {
a = 0
})
})

o.beforeEach(function() {
return wrapPromise(function() {
b = 1
})
})
o.afterEach(function() {
return wrapPromise(function() {
b = 0
})
})

o("promise functions", async function() {
await wrapPromise(function() {
o(a).equals(b)
o(a).equals(1)("a and b should be initialized")
})
})
})
})
2 changes: 1 addition & 1 deletion promise/tests/test-promise.js
Expand Up @@ -131,7 +131,7 @@ o.spec("promise", function() {
callAsync(function() {resolve(promise)})
})

promise.then(null, done)
promise.then(null, () => done())
Copy link
Member

Choose a reason for hiding this comment

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

My one and only nit: use ES5-style functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

@isiahmeadows only here or everywhere?

Copy link
Member

Choose a reason for hiding this comment

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

For this, only in places where you have a diff. The rest should be flagged in a separate issue, if you'd like to file one for that. (The polyfill tests should be runnable in an ES5 environment for obvious reasons.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, I have some other arrows in this PR. Should I convert them too? I assume yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, IMO tests are fine to use arrows but library has to use ES5 funcs for compat w/o transpilation.

This sounds like a good danger rule... 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

I converted one arrow fn in ospec to a fn-expression. Left the tests untouched.

})
o("non-function onFulfilled is ignored", function(done) {
var promise = Promise.resolve(1)
Expand Down