Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

util fake-fetch #8363

Merged
merged 8 commits into from Apr 11, 2018
Merged

util fake-fetch #8363

merged 8 commits into from Apr 11, 2018

Conversation

niklasad1
Copy link
Collaborator

Attempt to close #8240

But I haven't fixed yet dapps/FakeFetch because it has more functionality but I want to know whether this is on the right track or not before spending more time on it!

Also, the tests in price_info need to be re-factored because they might pass even if the closures are not ever executed, https://github.com/paritytech/parity/blob/master/price-info/src/lib.rs#L216-#L255

/cc @debris

@niklasad1 niklasad1 added A0-pleasereview 🤓 Pull request needs code review. A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. labels Apr 11, 2018
Copy link
Collaborator

@debris debris left a comment

Choose a reason for hiding this comment

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

you are definitely on the right track! imo we could merge it as it is. @tomusdrw can you confirm?

@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 11, 2018
Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Looks good!


fn fetch(&self, request: Request, abort: fetch::Abort) -> Self::Result {
let u = request.url().clone();
let (tx, rx) = futures::oneshot();
thread::spawn(move || {
future::ok(if self.val.is_some() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think thread was introduced here to simulate some asynchronicity (I remember fixing an issue related to this). But I think it's fine as is anyway.

@debris debris merged commit 9fe991d into master Apr 11, 2018
@debris debris deleted the fake-fetch branch April 11, 2018 09:59
@5chdn 5chdn added this to the 1.11 milestone Apr 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unify FakeFetch/TestFetch
4 participants