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

fixed tests, now they are broken #150

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
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
64 changes: 26 additions & 38 deletions src/ember-app.js
Original file line number Diff line number Diff line change
Expand Up @@ -226,21 +226,31 @@ class EmberApp {
* @param {Object} result
* @return {Promise<instance>} instance
*/
visitRoute(path, fastbootInfo, bootOptions, result) {
let instance;

visitRoute(path, fastbootInfo, bootOptions, result, destroyAppInstanceInMs) {
return this.buildAppInstance()
.then(appInstance => {
instance = appInstance;
.then(instance => {
result.instance = instance;
registerFastBootInfo(fastbootInfo, instance);
return new Promise((resolve, reject) => {
let timer;
if (destroyAppInstanceInMs > 0) {
// start a timer to destroy the appInstance forcefully in the given ms.
// This is a failure mechanism so that node process doesn't get wedged if the `visit` never completes.
timer = setTimeout(() => {
if (result._destroyAppInstance()) {
reject(new Error('Fastboot forcefully destroyed App instance in ' + destroyAppInstanceInMs + 'ms'));
}
}, destroyAppInstanceInMs);
}

return instance.boot(bootOptions);
})
.then(() => result.instanceBooted = true)
.then(() => instance.visit(path, bootOptions))
.then(() => fastbootInfo.deferredPromise)
.then(() => instance);
registerFastBootInfo(fastbootInfo, instance);
instance.boot(bootOptions)
.then(() => result.instanceBooted = true)
.then(() => result.instance.visit(path, bootOptions))
.then(() => fastbootInfo.deferredPromise)
.then(() => clearTimeout(timer))
.then(resolve, reject);
});
});
}

/**
Expand Down Expand Up @@ -275,31 +285,15 @@ class EmberApp {
let shouldRender = (options.shouldRender !== undefined) ? options.shouldRender : true;
let bootOptions = buildBootOptions(shouldRender);
let fastbootInfo = new FastBootInfo(
req,
res,
req, res,
{ hostWhitelist: this.hostWhitelist, metadata: options.metadata }
);

let doc = bootOptions.document;

let result = new Result({
doc: doc,
html: html,
fastbootInfo: fastbootInfo
});
let result = new Result({ doc, html, fastbootInfo });

let destroyAppInstanceTimer;
if (destroyAppInstanceInMs > 0) {
// start a timer to destroy the appInstance forcefully in the given ms.
// This is a failure mechanism so that node process doesn't get wedged if the `visit` never completes.
destroyAppInstanceTimer = setTimeout(function() {
if (result._destroyAppInstance()) {
result.error = new Error('App instance was forcefully destroyed in ' + destroyAppInstanceInMs + 'ms');
}
}, destroyAppInstanceInMs);
}

return this.visitRoute(path, fastbootInfo, bootOptions, result)
return this.visitRoute(path, fastbootInfo, bootOptions, result, destroyAppInstanceInMs)
.then(() => {
if (!disableShoebox) {
// if shoebox is not disabled, then create the shoebox and send API data
Expand All @@ -308,13 +302,7 @@ class EmberApp {
})
.catch(error => result.error = error)
.then(() => result._finalize())
.finally(() => {
if (result._destroyAppInstance()) {
if (destroyAppInstanceTimer) {
clearTimeout(destroyAppInstanceTimer);
}
}
});
.finally(() => result._destroyAppInstance());
}

/**
Expand Down
1 change: 1 addition & 0 deletions src/result.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ class Result {
* to this Result instance.
*/
_finalize() {
if (this._instanceDestroyed) { return this; }
if (this.finalized) {
throw new Error("Results cannot be finalized more than once");
}
Expand Down
3 changes: 2 additions & 1 deletion test/.jshintrc
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{
"mocha": true,
"esnext": true,
"node": true
"node": true,
"expr": true
}
25 changes: 14 additions & 11 deletions test/fastboot-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,16 +101,15 @@ describe("FastBoot", function() {
});
});

it("can forcefully destroy the app instance using destroyAppInstanceInMs", function() {
it("can forcefully destroy the app instance using destroyAppInstanceInMs", function(done) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because otherwise it is skipping assertion in catch, you can try yourself

Copy link
Contributor

Choose a reason for hiding this comment

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

I a bit surprised with the mocha behavior if it is right. I thought returning promise or done was the same in mocha.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be fixed in mocha upstream as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think since you are returning resolved promise it is behaving correctly, it either needs rejections inside then or it needs to use done

I think this should be fixed in mocha upstream as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

In general I find https://github.com/domenic/chai-as-promised clears all this up nicely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will cleanup with next PR, using it :)

var fastboot = new FastBoot({
distPath: fixture('basic-app')
distPath: fixture('long-resolving-app')
});

return fastboot.visit('/', {
destroyAppInstanceInMs: 5
})
fastboot.visit('/', { destroyAppInstanceInMs: 5 })
.catch((e) => {
expect(e.message).to.equal('App instance was forcefully destroyed in 5ms');
expect(e.message).to.equal('Fastboot forcefully destroyed App instance in 5ms');
Copy link
Contributor

Choose a reason for hiding this comment

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

if this expect fails, I believe an assertion is thrown preventing done from being invoked. This may result in annoying test issues on failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test will just timeout, current timeout 1000ms I think, so it will still fail,
but if I return whole promsie return fastboot.visit('/', latest mocha will complain that
we are returning promise and using done at the same time, thoughts ?

Copy link
Contributor

Choose a reason for hiding this comment

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

So why can't we just return the promise and move the assertion in the reject function?

Copy link
Contributor Author

@bekzod bekzod Jun 10, 2017

Choose a reason for hiding this comment

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

@kratiahuja because of this #150 (comment) , the whole point of this PR was not to do that, because of mentioned problems :)

done();
});
});

Expand Down Expand Up @@ -158,14 +157,15 @@ describe("FastBoot", function() {
return expect(fastboot.visit('/')).to.be.rejected;
});

it("catches the error if an error occurs", function() {
it("catches the error if an error occurs", function(done) {
var fastboot = new FastBoot({
distPath: fixture('rejected-promise')
});

fastboot.visit('/')
.catch(function(err) {
return expect(err).to.be.not.null;
expect(err).to.be.not.null;
done();
});
});

Expand Down Expand Up @@ -344,13 +344,16 @@ describe("FastBoot", function() {
}
});

it("handles apps boot-time failures by throwing Errors", function() {
it("handles apps boot-time failures by throwing Errors", function(done) {
var fastboot = new FastBoot({
distPath: fixture('boot-time-failing-app')
});

return fastboot.visit('/')
.catch((e) => expect(e).to.be.an('error'));
fastboot.visit('/')
.catch((e) => {
expect(e).to.be.an('error');
done();
});
});

it("matches app's fastboot-info and result's fastboot-info", function() {
Expand Down
Loading