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

Change test strings for clearer picture of what's going on #48

Closed
wants to merge 3 commits into from
Closed

Change test strings for clearer picture of what's going on #48

wants to merge 3 commits into from

Conversation

sdhull
Copy link
Contributor

@sdhull sdhull commented Jul 10, 2019

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 in fastboot-app-server tests (even after updating fastboot-app-server to properly pass resilient through).

Copy link
Member

@rwjblue rwjblue left a 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?

@sdhull
Copy link
Contributor Author

sdhull commented Jul 11, 2019

@rwjblue

.travis.yml Outdated
@@ -1,8 +1,9 @@
language: node_js
node_js:
- "stable"
- "4"
Copy link
Member

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.

@sdhull
Copy link
Contributor Author

sdhull commented Jul 12, 2019

@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?

@rwjblue
Copy link
Member

rwjblue commented Jul 12, 2019

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)

@sdhull
Copy link
Contributor Author

sdhull commented Jul 12, 2019

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"?

Copy link
Member

rwjblue commented Jul 12, 2019

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.
@sdhull
Copy link
Contributor Author

sdhull commented Jul 16, 2019

@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

@sdhull
Copy link
Contributor Author

sdhull commented Jul 16, 2019

@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 rejected-promise fixture so that the test case can truly demonstrate that it does not render any markup from Ember. Right now it just demonstrates that it doesn't render an error but rather the raw index.html file

@ghost ghost closed this by deleting the head repository Jan 31, 2023
This pull request was closed.
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.

2 participants