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

The networking code is hard to test #6727

Closed
jdm opened this issue Jul 24, 2015 · 6 comments
Closed

The networking code is hard to test #6727

jdm opened this issue Jul 24, 2015 · 6 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Jul 24, 2015

We can write full integration tests in the form of pages that run in the WPT harness and Python scripts that control the server responses, but it's not particularly efficient. At some point we're going to implement an HTTP cache, and being able to test precisely how the networking stack treats a series of HTTP requests/responses will be important. Furthermore, the improvements for testing can lay the groundwork for an implementation of request interception for a future ServiceWorker implementation!

I propose the following:

  • we create a new Response struct that contains all of the data used by Servo's networking stack from hyper's Response
  • we create a trait called HTTPResponseProvider with a method that takes in all the data associated with an HTTP request and returns the new Response type
  • we create an implementation called RemoteHTTPResponse that extracts all of the code from http_loader::load that initiates a network connection and waits for a response
  • we figure out what else makes http_loader::load difficult to test
@jdm
Copy link
Member Author

@jdm jdm commented Jul 24, 2015

cc @samfoo does this address your concerns from #6703?

@samfoo
Copy link
Contributor

@samfoo samfoo commented Jul 26, 2015

Yep, it's a great start.

@samfoo
Copy link
Contributor

@samfoo samfoo commented Jul 30, 2015

Hey guys, I'll pick this up over the weekend.

@jdm
Copy link
Member Author

@jdm jdm commented Jul 30, 2015

Fantastic!

samfoo added a commit to samfoo/servo that referenced this issue Aug 2, 2015
This simplifies the arguments that are passed in and should make testing
errors/responses easier once they're mocked

servo#6727
samfoo added a commit to samfoo/servo that referenced this issue Aug 2, 2015
samfoo added a commit to samfoo/servo that referenced this issue Aug 2, 2015
@samfoo
Copy link
Contributor

@samfoo samfoo commented Aug 2, 2015

I've done some proof-of-concept work on this over the past couple of days. I'm still debating whether the approach makes sense. The current approach:

  • Abstract load into two different functions, one that is responsible for performing the request and managing errors, and one that handles IPC/inter-task comms. This, at least, makes testing less complicated.
  • Inject the hyper::net::NetworkConnector dependency, and then mock that out (as necessary) in tests. For the proof-of-concept, I've simply copied all the HTTP1 code from hyper::mock. It's probably better to wrap this in something like what @jdm suggested, like a servo HttpRequester or something, but the current code is pretty tightly coupled to hyper at the moment.
  • Written two tests just to show some progress 🎁.

I've also made some simplifying assumptions for the moment, that may need to change before finishing (e.g. is nossl still an option that we care about? are all unsupported scheme errors are the same? etc)

@jdm
Copy link
Member Author

@jdm jdm commented Aug 2, 2015

Exciting! I am having trouble imagining how the split into two functions was managed, but perhaps I'm over-complicating matters in my head.

samfoo added a commit to samfoo/servo that referenced this issue Aug 9, 2015
Because we're using unsized types not for requesting, there's not a
satisfactory way of doing this without boxing the request...

Once unsized stuff lands in rust 1.2/1.3(???) then this should be
implemented with Rc's instead of Box's.

For the time being I'm not sure what else to do.

servo#6727
samfoo added a commit to samfoo/servo that referenced this issue Aug 9, 2015
This simplifies the arguments that are passed in and should make testing
errors/responses easier once they're mocked

servo#6727
samfoo added a commit to samfoo/servo that referenced this issue Aug 9, 2015
samfoo added a commit to samfoo/servo that referenced this issue Aug 9, 2015
samfoo added a commit to samfoo/servo that referenced this issue Aug 9, 2015
Because we're using unsized types not for requesting, there's not a
satisfactory way of doing this without boxing the request...

Once unsized stuff lands in rust 1.2/1.3(???) then this should be
implemented with Rc's instead of Box's.

For the time being I'm not sure what else to do.

servo#6727
samfoo added a commit to samfoo/servo that referenced this issue Aug 9, 2015
The HttpRequest trait doesn't make sense, on further reflection. Rather,
just modify the method signature on the requester. The hyper request was
only being used to mutate it's headers anyway.

servo#6727
samfoo added a commit to samfoo/servo that referenced this issue Aug 10, 2015
samfoo added a commit to samfoo/servo that referenced this issue Aug 15, 2015
This simplifies the arguments that are passed in and should make testing
errors/responses easier once they're mocked

servo#6727
samfoo added a commit to samfoo/servo that referenced this issue Aug 15, 2015
samfoo added a commit to samfoo/servo that referenced this issue Aug 15, 2015
samfoo added a commit to samfoo/servo that referenced this issue Aug 15, 2015
Because we're using unsized types not for requesting, there's not a
satisfactory way of doing this without boxing the request...

Once unsized stuff lands in rust 1.2/1.3(???) then this should be
implemented with Rc's instead of Box's.

For the time being I'm not sure what else to do.

servo#6727
samfoo added a commit to samfoo/servo that referenced this issue Aug 24, 2015
@jdm jdm added the C-assigned label Aug 26, 2015
samfoo added a commit to samfoo/servo that referenced this issue Aug 27, 2015
This simplifies the arguments that are passed in and should make testing
errors/responses easier once they're mocked

servo#6727
samfoo added a commit to samfoo/servo that referenced this issue Aug 27, 2015
samfoo added a commit to samfoo/servo that referenced this issue Aug 27, 2015
samfoo added a commit to samfoo/servo that referenced this issue Aug 27, 2015
Because we're using unsized types not for requesting, there's not a
satisfactory way of doing this without boxing the request...

Once unsized stuff lands in rust 1.2/1.3(???) then this should be
implemented with Rc's instead of Box's.

For the time being I'm not sure what else to do.

servo#6727
samfoo added a commit to samfoo/servo that referenced this issue Aug 27, 2015
The HttpRequest trait doesn't make sense, on further reflection. Rather,
just modify the method signature on the requester. The hyper request was
only being used to mutate it's headers anyway.

servo#6727
samfoo added a commit to samfoo/servo that referenced this issue Aug 27, 2015
bors-servo pushed a commit that referenced this issue Aug 28, 2015
Testable net load...

*The goal of this PR is to get early feedback on this before I go too far down the rabbit hole. This new code path is working, and there's several tests I've written as a proof of concept. There are still some regressions that I'll be fixing in the coming days.*

I've abstracted out the request/response cycle so that it's no longer dependent on the Hyper request/response structs. Since request/response @ hyper are structs, not traits, it made mocking them for tests impossible.

Current issues/concerns:

* This relies on boxing the `HttpResponse` that gets returned from the `HttpRequester` because `HttpResponse` is unsized. I don't know if there's a more idiomatic rust-y way of doing this?
* This relies on boxing the `Read` that is now returned from `load` for the same reason.
* The devtools and resource manager channels are still passed into `load`. It might be easier to inject these as trait dependencies instead of chans as well?
* Needs more tests.

🎩 

#6727

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7139)
<!-- Reviewable:end -->
samfoo added a commit to samfoo/servo that referenced this issue Aug 30, 2015
This simplifies the arguments that are passed in and should make testing
errors/responses easier once they're mocked

servo#6727
samfoo added a commit to samfoo/servo that referenced this issue Aug 30, 2015
samfoo added a commit to samfoo/servo that referenced this issue Aug 30, 2015
samfoo added a commit to samfoo/servo that referenced this issue Aug 30, 2015
Because we're using unsized types not for requesting, there's not a
satisfactory way of doing this without boxing the request...

Once unsized stuff lands in rust 1.2/1.3(???) then this should be
implemented with Rc's instead of Box's.

For the time being I'm not sure what else to do.

servo#6727
samfoo added a commit to samfoo/servo that referenced this issue Aug 30, 2015
The HttpRequest trait doesn't make sense, on further reflection. Rather,
just modify the method signature on the requester. The hyper request was
only being used to mutate it's headers anyway.

servo#6727
samfoo added a commit to samfoo/servo that referenced this issue Aug 30, 2015
bors-servo pushed a commit that referenced this issue Aug 30, 2015
Testable net load...

*The goal of this PR is to get early feedback on this before I go too far down the rabbit hole. This new code path is working, and there's several tests I've written as a proof of concept. There are still some regressions that I'll be fixing in the coming days.*

I've abstracted out the request/response cycle so that it's no longer dependent on the Hyper request/response structs. Since request/response @ hyper are structs, not traits, it made mocking them for tests impossible.

Current issues/concerns:

* This relies on boxing the `HttpResponse` that gets returned from the `HttpRequester` because `HttpResponse` is unsized. I don't know if there's a more idiomatic rust-y way of doing this?
* This relies on boxing the `Read` that is now returned from `load` for the same reason.
* The devtools and resource manager channels are still passed into `load`. It might be easier to inject these as trait dependencies instead of chans as well?
* Needs more tests.

🎩 

#6727

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7139)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Aug 31, 2015
Testable net load...

*The goal of this PR is to get early feedback on this before I go too far down the rabbit hole. This new code path is working, and there's several tests I've written as a proof of concept. There are still some regressions that I'll be fixing in the coming days.*

I've abstracted out the request/response cycle so that it's no longer dependent on the Hyper request/response structs. Since request/response @ hyper are structs, not traits, it made mocking them for tests impossible.

Current issues/concerns:

* This relies on boxing the `HttpResponse` that gets returned from the `HttpRequester` because `HttpResponse` is unsized. I don't know if there's a more idiomatic rust-y way of doing this?
* This relies on boxing the `Read` that is now returned from `load` for the same reason.
* The devtools and resource manager channels are still passed into `load`. It might be easier to inject these as trait dependencies instead of chans as well?
* Needs more tests.

🎩 

#6727

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7139)
<!-- Reviewable:end -->
@jdm jdm closed this Sep 17, 2015
josiahdaniels added a commit to josiahdaniels/servo that referenced this issue Sep 28, 2015
This simplifies the arguments that are passed in and should make testing
errors/responses easier once they're mocked

servo#6727
josiahdaniels added a commit to josiahdaniels/servo that referenced this issue Sep 28, 2015
josiahdaniels added a commit to josiahdaniels/servo that referenced this issue Sep 28, 2015
josiahdaniels added a commit to josiahdaniels/servo that referenced this issue Sep 28, 2015
Because we're using unsized types not for requesting, there's not a
satisfactory way of doing this without boxing the request...

Once unsized stuff lands in rust 1.2/1.3(???) then this should be
implemented with Rc's instead of Box's.

For the time being I'm not sure what else to do.

servo#6727
josiahdaniels added a commit to josiahdaniels/servo that referenced this issue Sep 28, 2015
The HttpRequest trait doesn't make sense, on further reflection. Rather,
just modify the method signature on the requester. The hyper request was
only being used to mutate it's headers anyway.

servo#6727
josiahdaniels added a commit to josiahdaniels/servo that referenced this issue Sep 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.