-
Notifications
You must be signed in to change notification settings - Fork 18
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
Browsersync second round #20
Conversation
expect.use(require('unexpected-http')) | ||
.use(require('unexpected-image')) | ||
.use(require('unexpected-resemble')) | ||
.use(require('unexpected-sinon')) | ||
.use(require('magicpen-prism')) | ||
.addAssertion('<string> to respond with <object|number>', function (expect, subject, value) { | ||
var modifiedSubject = subject.replace(' ', ' http://localhost:9999'); | ||
.addAssertion('<string|object> to respond with <object|number>', function (expect, subject, value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would read or document itself better if it was a middle-of-the-rocket assertion, something like expect('GET /foo', 'prepended with hostname', 'to yield response', 200)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it's a bit weird.
I think I would actually prefer to use the vanilla to yield response satisfying
assertion (unexpected-http) and just put http://localhost:${serverPort}
into the url in each test. That would make it easier to realize that those tests connect to a "real" server instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make that change
@papandreou @joelmukuthu How about now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :)
Released in v7.1.1 |
res.status
needs to be polyfilled for browser-sync support