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

util fake-fetch#8363

Merged
debris merged 8 commits into
masterfrom
fake-fetch
Apr 11, 2018
Merged

util fake-fetch#8363
debris merged 8 commits into
masterfrom
fake-fetch

Conversation

@niklasad1
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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!

let u = request.url().clone();
let (tx, rx) = futures::oneshot();
thread::spawn(move || {
future::ok(if self.val.is_some() {
Copy link
Copy Markdown
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