Closed Bug 1188644 Opened 4 years ago Closed 4 years ago

Use channel->ascynOpen2 in netwerk/test

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
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
Assignee: nobody → mozilla
Blocks: 1182535
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
Attachment #8643930 - Flags: review?(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
Attachment #8643930 - Flags: review?(mcmanus)
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-------
Attachment #8643930 - Attachment is obsolete: true
Attachment #8645024 - Flags: review?(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.
Attachment #8645024 - Flags: review?(mcmanus) → review+
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/a647ab8f8d8d5808980a1d28f28f26e2f0d910c8
changeset:  a647ab8f8d8d5808980a1d28f28f26e2f0d910c8
user:       Christoph Kerschbaumer <mozilla@christophkerschbaumer.com>
date:       Mon Aug 10 10:20:40 2015 -0700
description:
Bug 1188644 - Use channel->ascynOpen2 in netwerk/test (r=mcmanus)
https://hg.mozilla.org/mozilla-central/rev/a647ab8f8d8d
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.