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

fix: Use ember-fetch to avoid test failures #200

Closed
wants to merge 2 commits into from

Conversation

knownasilya
Copy link
Contributor

@knownasilya knownasilya commented May 18, 2017

closes #187

I'm not sure how to stub fetch, and fix the tests. Any guidance is appreciated.

Maybe something like http://www.wheresrhys.co.uk/fetch-mock/ ?

});
body: JSON.stringify(data),
headers: modifiedHeaders
}).then((response) => response.json());
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldnt we want to return the response?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just kidding, that does return it.

@knownasilya
Copy link
Contributor Author

Also, I'd like to confirm that this PR does solve the original issue.

@jpadilla
Copy link
Contributor

jpadilla commented May 18, 2017

@knownasilya thanks for tackling this! fetch-mock looks like the simplest solution to me. Also, it seems like tests are failing in Travis due to some other thing unrelated to this PR, gah!

@tomclose
Copy link

tomclose commented Sep 3, 2017

I ran into this issue too. It makes it impossible to write robust acceptance tests that cover login and took a while to debug. It would be great to get this fix merged, so that others won't hit the same issue.

@knownasilya I've put up a PR that fixes the tests.

@jpadilla Is that all that's preventing this PR from being merged?

@TopsyCM
Copy link

TopsyCM commented Jan 6, 2018

I just set up a fork to implement this same fix. What needs doing to get this merged? I see the PR that @tomclose put up has a conflict, but is that all?

@jpadilla
Copy link
Contributor

Hey y'all! Sorry for the delay here. With v3 recently out the door, would @knownasilya or anyone else want to pickup this up to move it forward?

@knownasilya
Copy link
Contributor Author

I don't currently have time to work on the tests for this but can merge in any changes into my branch to continue the work.

@jpadilla
Copy link
Contributor

Closed by #226. Thanks again ya'll!

@jpadilla jpadilla closed this May 24, 2018
@knownasilya knownasilya deleted the use-fetch branch May 24, 2018 00:51
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.

5 participants