1188644 - Use channel->ascynOpen2 in netwerk/test

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

All these tests can land together within one patch:

/netwerk/test/TestCallbacks.cpp
/netwerk/test/TestHttp.cpp
/netwerk/test/TestProtocols.cpp
/netwerk/test/TestStreamLoader.cpp
/netwerk/test/TestUpload.cpp
/netwerk/test/TestRes.cpp
/netwerk/test/TestStreamChannel.cpp

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1188644_asyncopen2_netwerk_test.patch

Pat, here are all the netwerk tests we are about to update to use asyncOpen2() with a little description.

> /netwerk/test/TestCallbacks.cpp
AsyncOpen(2) only takes one argument. Is it ok to just remove contextSup? Test still passes. 

> /netwerk/test/TestHttp.cpp
not compiled at the moment [1] - do you want me to remove that test?

> /netwerk/test/TestProtocols.cpp
passes a URLLoadInfo as the second argument for asyncOpen(2), not sure if we can 'just' remove that second argument or if that makes the test more or less useless.

> /netwerk/test/TestStreamLoader.cpp
> /netwerk/test/TestUpload.cpp
those two should be fine.

> /netwerk/test/TestRes.cpp
not compiled at the moment; uses a listener for the second argument in AsyncOpen().

> /netwerk/test/TestStreamChannel.cpp
not compiled, commented out [1]

Please let me know how you would like me to proceed with those tests.

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/test/moz.build#20

Comment 3

[Patrick McManus [:mcmanus]]

Thanks. These tests have limited value so let's not beat ourselves up about them

 
> > /netwerk/test/TestCallbacks.cpp
> AsyncOpen(2) only takes one argument. Is it ok to just remove contextSup?
> Test still passes. 

that's weird. The Validate() code tries to make sure that the aContext provided in OnStartRequest is the same as contextSup. If you just removed the argument I would think it would fail..

> 
> > /netwerk/test/TestHttp.cpp
> not compiled at the moment [1] - do you want me to remove that test?
> 
yes remove, thanks

> > /netwerk/test/TestProtocols.cpp
> passes a URLLoadInfo as the second argument for asyncOpen(2), not sure if we
> can 'just' remove that second argument or if that makes the test more or
> less useless.
> 

I think we need that.. but it looks like it can just become a member of InputTestConsumer easily enough

> 
> > /netwerk/test/TestRes.cpp
> not compiled at the moment; uses a listener for the second argument in
> AsyncOpen().

just delete it if its not compiled.

> 
> > /netwerk/test/TestStreamChannel.cpp
> not compiled, commented out [1]

same

> 
> Please let me know how you would like me to proceed with those tests.
> 
> [1] http://mxr.mozilla.org/mozilla-central/source/netwerk/test/moz.build#20

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1188644_asyncopen2_netwerk_test.patch

Hey Pat, thanks for the feedback. Here is what I propose for each individual testcase. To be honest, I do not want to put too much effort into updating those tests at the moment.

> netwerk/test/TestStreamLoader.cpp
> netwerk/test/TestUpload.cpp
Those two were easy to update, so I converted them from using asyncOpen() to use asyncOpen(2).


> netwerk/test/TestProtocols.cpp
> I think we need that.. but it looks like it can just become a member of
> InputTestConsumer easily enough
I propose to keep that test 'as is' till we are completely done with converting all the callsites from asyncOpen() to use asyncOpen(2). Once we are in the process of removing asyncOpen() completely from our codebase, we can re-evaluate what we do with that test. Sounds good? JFYI, at the moment we have converted 17 out of 87 callsites in C++ and 0 out of 340 callsites in JS :-)


> netwerk/test/TestHttp.cpp
> netwerk/test/TestRes.cpp
> netwerk/test/TestStreamChannel.cpp
> netwerk/test/TestCallbacks.cpp
I deleted those 4 tests including TestCallbacks.cpp since it was not working properly anyway.

> that's weird. The Validate() code tries to make sure that the aContext provided
> in OnStartRequest is the same as contextSup. If you just removed the argument
> I would think it would fail..

The tests prints an error [see undeneath], but it still passes. If you prefer, we could also keep that test around 'as is', but I thought since it's not working properly anyway, there is no need in keeping it. Up to you.


Consumer::OnStart() -> in

ERROR: Contexts do not match
Consumer::OnStop() -> in

INFO: received 0 OnData()s
ERROR: Contexts do not match
Consumer::~Consumer -> in

Consumer::~Consumer -> out

-------ERROR-------

Comment 5

[Patrick McManus [:mcmanus]]

Comment on attachment 8645024 [details] [diff] [review]
bug_1188644_asyncopen2_netwerk_test.patch

Review of attachment 8645024 [details] [diff] [review]:
-----------------------------------------------------------------

I think your plan is fine for these tests except for TestCallbacks. We should just remove that test - mostly it is supposed to be testing that second arg that we are deprecating anyhow.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/a647ab8f8d8d5808980a1d28f28f26e2f0d910c8
changeset:  a647ab8f8d8d5808980a1d28f28f26e2f0d910c8
user:       Christoph Kerschbaumer <[e-mail]>
date:       Mon Aug 10 10:20:40 2015 -0700
description:
Bug 1188644 - Use channel->ascynOpen2 in netwerk/test (r=mcmanus)

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a647ab8f8d8d