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

Introducing "it.yield", a convenient way to test promise code #9601

Merged
merged 4 commits into from
Jun 4, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions test/_init_tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
} from '../src/error';
import {resetExperimentTogglesForTesting} from '../src/experiments';
import * as describes from '../testing/describes';
import {installYieldIt} from '../testing/yield';
import stringify from 'json-stable-stringify';


Expand Down Expand Up @@ -207,6 +208,8 @@ describe.configure = function() {
return new TestConfig(describe);
};

installYieldIt(it);

it.configure = function() {
return new TestConfig(it);
};
Expand Down
104 changes: 104 additions & 0 deletions test/functional/test-yield.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
/**
* Copyright 2017 The AMP HTML Authors. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS-IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import * as lolex from 'lolex';

describes.realWin('yield', {}, env => {

let win;
let clock;

beforeEach(() => {
win = env.win;
clock = lolex.install(win, 0, ['Date', 'setTimeout', 'clearTimeout']);
});

it('should work with nested promises', function* () {
let value = false;

const nestPromise = level => {
if (level == 0) {
value = true;
return;
}
return Promise.resolve().then(() => {
return nestPromise(level - 1);
});
};

nestPromise(100);
expect(value).to.be.false;
yield;
expect(value).to.be.true;
});

it('should work with promise chain', function* () {
let value;

const chainPromise = Promise.resolve();
for (let i = 0; i < 100; i++) {
chainPromise.then(() => {value = false;});
}
chainPromise.then(() => {
value = true;
});
expect(value).to.be.undefined;
yield;
expect(value).to.be.true;
});

it('should work with promise inside setTimeout', function* () {
let value;
win.setTimeout(() => {
value = false;
Promise.resolve().then(() => {
value = true;
});
}, 100);

expect(value).to.be.undefined;
clock.tick(100);
expect(value).to.be.false;
yield;
expect(value).to.be.true;
});

it('should work with manually resolved promise inside ' +
'setTimeout', function* () {
let value;
let resolver;
const promise = new Promise(r => {resolver = r;});
promise.then(() => {
value = true;
});
win.setTimeout(() => {
value = false;
resolver();
}, 100);
clock.tick(100);
expect(value).to.be.false;
yield;
expect(value).to.be.true;
});

it('should block a promise', function* () {
let resolver;
const promise = new Promise(r => {resolver = r;}).then(() => 'yes');
resolver();
const result = yield promise;
expect(result).to.equal('yes');
});
});
67 changes: 67 additions & 0 deletions testing/yield.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/**
* Copyright 2017 The AMP HTML Authors. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS-IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/**
* Install "yield" support to Mocha tests.
* Check test-yield.js for how-to.
*/
export function installYieldIt(realIt) {
it = enableYield.bind(null, realIt); // eslint-disable-line no-native-reassign, no-undef
Copy link
Contributor

Choose a reason for hiding this comment

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

global.it = .... we could do it.yields?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can do either:

it.yields('message', function*() {
});

or

it('message', function*() {
});

I'm in favor of the latter because it makes the other syntax like it.only and it.skip unchanged.

it./*OK*/only = enableYield.bind(null, realIt.only);
it.skip = realIt.skip;
}

function enableYield(fn, message, runnable) {
if (!runnable || !runnable.constructor
|| runnable.constructor.name !== 'GeneratorFunction') {
return fn(message, runnable);
}
return fn(message, done => {
const runner = (iterator, result) => {
let state;
try {
state = iterator.next(result);
} catch (e) {
// catch any assertion errors and pass to `done`
// otherwise the messages are swallowed
return done(e);
}
if (state.done) {
return done();
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to wait for the state's value to complete.

if (state.done) {
  Promise.resolve(state.value).then(() => done(), done);
  return;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

}

const _runner = runner.bind(null, iterator);
if (isPromise(state.value)) {
// With this, we can do: `const result = yield promise;`
state.value.then(_runner).catch(done);
Copy link
Contributor

Choose a reason for hiding this comment

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

This catch clause is incorrect. If a Promise is yielded, and it rejects, it should be passed into Generator#throw. Also, it can be in the same then block, not after in a catch block:

state.value.then(_runner, (error) => {
  // this will basically duplicate everything in runner
  try {
    result = iterator.throw(error);
  } catch (e) {
    return done(e);
  }
  //...
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I still don't quite understand this part. Why is it necessary to throw the error to iterator, instead of just finish the test case immediately?

I think it's harmless to have an iterator hang there without iterating to the end, no? Or is it for better error reporting?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not for error reporting, but so we can take full advantage of the generator's functionality. Imagine:

it('regular code', function() {
  return somePromiseThatRejects.then(() => {
    throw new Error('UNREACHABLE');
  }, err => {
    expect(err.message).to.contain('something');
  });
});

As it is now, this cannot be written into our yield style, because the promise would reject and kill the test. Instead, we want something like:

it('generator yield', function*() {
  try {
    yield somePromiseThatRejects;
    throw new Error('UNREACHABLE');
  } catch (e) {
    expect(e.message).to.contain('something');
  }
});

That's throwing the error into the iterator, allowing us to test all our promise tests as sync generators.

} else {
// With this, we can do: `yield 50;`, which blocks the test for 50ms
// We should rarely need this in unit test, use with caution, as it
// usually brings test flakiness.
const timeout = (typeof state.value === 'number') ? state.value : 0;
setTimeout(_runner, timeout);
Copy link
Contributor

Choose a reason for hiding this comment

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

Still strongly against this.

}
};
runner(runnable());
});
}

function isPromise(subject) {
if (subject === undefined || subject === null) {
return false;
}
return typeof subject.then == 'function';
}