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

Extending middleware functionality, and documenting in config_file #1053

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

alexander-alvarez
Copy link

Closes #1052

@johanneswuerbach
Copy link
Member

johanneswuerbach commented Jan 7, 2017

Could you add a test here https://github.com/testem/testem/blob/master/tests/server_tests.js to ensure we don't regress this feature in the future? Otherwise this looks good 👍

@johanneswuerbach
Copy link
Member

Friendly ping @alexander-alvarez :-)

@alexander-alvarez
Copy link
Author

👍 I'll get to it this long weekend

@alexander-alvarez
Copy link
Author

@johanneswuerbach can you check that this is testing in the right way?

after(function(done) {
server.stop(function() {
done();
});
Copy link
Member

Choose a reason for hiding this comment

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

Could you move this after code into the test itself? Otherwise if might be confusing when another test is added here and after kills the server for both.

});
server = new Server(config);
server.once('server-start', function() {
done();
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this test uses done twice?

Copy link
Author

Choose a reason for hiding this comment

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

modeled it after the auto port assignment, should I not do that?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but the done function should generally only be called once in a test. In your test you call it twice, ones after the server is booted and once during the middleware creation. I guess the middleware one should be dropped. Could you make sure that it no longer passes when you stop passing app in the code?

@alexander-alvarez
Copy link
Author

Could you make sure that it no longer passes when you stop passing app in the code?

@johanneswuerbach not sure what you meant

config = new Config('dev', {
port: 0,
cwd: 'tests',
middlewear: [
Copy link
Member

Choose a reason for hiding this comment

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

This property seems to contain a typo.

@NullVoxPopuli
Copy link

Would love to see some progress on this -- I'm trying to integrate testem with Vite, and it'd be a bit easier if I didn't need to figure out how to coordinate two servers 🙃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose server to middleware
3 participants