Make DataReductionProxy unit tests more integrated with net/ |
||||
Issue descriptionRight now quite a few tests in data_reduction_proxy/ rely on a specific order of execution in net/ as well as certain information available on URLRequest being a certain way. While these tests can be valuable for testing input vs expected effect, it would be better to also have holistic tests that actually run the net/ code (e.g., URLRequest->Start()). This is a fairly general pattern accross d_r_p/ tests and it would be good to add some more net/ tests. Ultimately, all code paths through each public method with every related input should be tested, but calling URLRequest->Start() and supplying responses to net/ should test the behavior of the d_r_p code as well.
,
Mar 14 2017
tbansal@ recently changed some unit tests to integrate with net more, so he might have more to say as well. Currently, the majority of DRP unit tests that involve NetworkDelegate or ProxyDelegate call the delegate methods directly. We should add tests that involve the complex logical flow involved in the net stack. We should have tests for d_r_p_network_delegate and d_r_p_delegate that call URLRequest->Start() with specific state on the URLRequest. The test should verify expected behavior from that. These tests should be supplemental to the unit tests and should be thought of as regression/integration tests WRT net/.
,
Mar 14 2017
I agree. Let's start there. We should remove direct calls to delegate methods from tests.
,
Mar 14 2017
I think having direct calls is valuable in addition to the non-directional approach.
,
Mar 14 2017
Re #1: I think at the minimum, we should add integration tests to ensure that caching works as expected. e.g., if the server provides a response that varies on CPAT header, then the test should ensure that a subsequent fetch would be served from cache. IIUC, the current set of unittests do not verify that caching works as expected, and so won't be able to catch any regressions. There are also a bunch of places where we add a header, and then subsequently remove it before the request goes on the wire. All those workflows can have proper integration tests.
,
Mar 14 2017
For reference, data_use_network_delegate_unittest.cc contains some of the integration tests: (1) DRP adds "br" to accept encoding header at the right places, so that Brotli requests are served from cache if there is a hit. (ii) DRP adds ECT header correctly, and if server varies on ECT, the caching behavior matches the expectations. (iii) Network stack does not return a decoding error on Brotli encoded responses for HTTP URLs.
,
Mar 15 2017
Ryan/Tarun, would you guys file specific bugs whenever you encounter a specific issue with our tests? This bug is too meta to determine when it is fixed. If helpful, you could leave this bug open and blocked on all of your specific bugs for tracking purposes.
,
Nov 8 2017
Please file specific bugs, if needed. This one is too vague as filed. |
||||
►
Sign in to add a comment |
||||
Comment 1 by bengr@chromium.org
, Feb 13 2017