-
Notifications
You must be signed in to change notification settings - Fork 42
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
Change test strings for clearer picture of what's going on #48
Conversation
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.
Seems good to me, mind poking at why CI is failing?
@rwjblue ✅ |
.travis.yml
Outdated
@@ -1,8 +1,9 @@ | |||
language: node_js | |||
node_js: | |||
- "stable" | |||
- "4" |
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 think we should make this change independently, and also update the engines
in package.json
. It'll require a major version bump.
I think we should support 8, 10, and >= 12.
@rwjblue master has been failing since @kratiahuja merged the update to Fastboot 2.x because Fastboot 2.x doesn't support Node 4.x How would you like to proceed? |
Basically like I mentioned above #48 (comment) bump .travis.yml and package.json's engines config in a separate PR (for easier inclusion in the changelog) |
OK I can do that. Should I bump the major version as well, considering that fastboot went from 1.x to 2.x as a result of "updating node.js support matrix"? |
No, I'll bump major's when I release |
It was unclear to me what scenarios would legitimately render a body that matched "error" vs when "hello world" was rendered because index.html was rendered or because the middleware was rendering that string directly. This changes index.html to contain the string "Original body" and the middleware to send the string "special error handling" to disambiguate the two. Additionally it adds some assertions around what the body _should_ match for examples that only had what it _should not_ match.
@rwjblue I've removed the commit that changes travis yml, but that means CI will fail until we merge #49 And I assume you're already aware but looks like @kratiahuja has already bumped major version yesterday |
@rwjblue I also just opened issue #50 to try to figure out why a couple of these test cases pass when afaict they shouldn't Also, I'm not sure how these fixtures work... I'd like to spruce up the |
It was unclear to me what scenarios would legitimately render a body that matched "error" vs when "hello world" was rendered because index.html was rendered or because the middleware was rendering that string directly.
This changes index.html to contain the string "Original body" and the middleware to send the string "special error handling" to disambiguate the two.
Additionally it adds some assertions around what the body should match for examples that only had what it should not match.
On a related note: I'm still trying to figure out why index.html is rendered fine with
resilient
enabled in these tests but not infastboot-app-server
tests (even after updating fastboot-app-server to properly passresilient
through).