Unit test for NULL values in preference validation delegate.

Unit test for NULL values in preference validation delegate.

BUG=384729
R=gab@chromium.org, mattm@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277852

[grt (UTC plus 2), 2014-06-15 02:34:53]

This isn't ready for review yet. I'll reply when it is. Cheers.

[grt (UTC plus 2), 2014-06-16 13:13:56]

and now it is. PTAL.

[grt (UTC plus 2), 2014-06-16 15:48:31]

i've added the comment requested by gab in
https://codereview.chromium.org/336983002/#msg5.

[gab, 2014-06-16 15:51:41]

https://codereview.chromium.org/332053005/diff/20001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/preference_validation_delegate_unittest.cc
(right):

https://codereview.chromium.org/332053005/diff/20001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/preference_validation_delegate_unittest.cc:126: //
An unknown type means a NULL value.
Specifically put -1 in a constant and handle it above; e.g. kNullValueForTest,
as this is essentially a hack for your test and it feels fairly obscure right
now.

https://codereview.chromium.org/332053005/diff/20001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/preference_validation_delegate_unittest.cc:142: if
(!*expected_value_) {
This is a fairly obscure way to do "check that the string is empty which is a
trick in this test to indicate a NULL value"; can you make this clearer to a the
next reader who will see it as a whole (and not in a diff like I am viewing it
now which clearly labels all the pieces of the hack)

https://codereview.chromium.org/332053005/diff/20001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/preference_validation_delegate_unittest.cc:154:
std::tr1::make_tuple(static_cast<base::Value::Type>(-1), ""),
"" and NULL value is not the same; I guess this is fine here, but please add
extra comments to explain this case.

[grt (UTC plus 2), 2014-06-16 16:56:05]

rejiggered for clarity. PTAL.

[mattm, 2014-06-16 19:52:15]

https://codereview.chromium.org/332053005/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/preference_validation_delegate_unittest.cc
(right):

https://codereview.chromium.org/332053005/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/preference_validation_delegate_unittest.cc:150:
TrackedPreferenceHelper::DONT_RESET);
add:
ASSERT_EQ(1U, incidents_.size())

https://codereview.chromium.org/332053005/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/preference_validation_delegate_unittest.cc:228:
EXPECT_EQ(1U, incidents_.size());
Should be ASSERT_EQ (same below)

[gab, 2014-06-16 22:33:27]

lgtm w/ optional comment to improve the reporting for NULL values.

Cheers,
Gab

https://codereview.chromium.org/332053005/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/preference_validation_delegate_unittest.cc
(right):

https://codereview.chromium.org/332053005/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/preference_validation_delegate_unittest.cc:94:
EXPECT_FALSE(incident->tracked_preference().has_atomic_value());
Optional: instead of returning no serialized value, you may want to return a
special string which you'll know means it was NULL server-side.

This may help in case you somehow start getting reports where the value is
somehow missing; you'd then be able to differentiate NULL values from broken
uploads.

[grt (UTC plus 2), 2014-06-17 02:05:33]

thanks. back to you two.

https://codereview.chromium.org/332053005/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/preference_validation_delegate_unittest.cc
(right):

https://codereview.chromium.org/332053005/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/preference_validation_delegate_unittest.cc:94:
EXPECT_FALSE(incident->tracked_preference().has_atomic_value());
On 2014/06/16 22:33:27, gab wrote:
> Optional: instead of returning no serialized value, you may want to return a
> special string which you'll know means it was NULL server-side.
> 
> This may help in case you somehow start getting reports where the value is
> somehow missing; you'd then be able to differentiate NULL values from broken
> uploads.

value_state() indicates that it was cleared. i've added this to the test.

https://codereview.chromium.org/332053005/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/preference_validation_delegate_unittest.cc:150:
TrackedPreferenceHelper::DONT_RESET);
On 2014/06/16 19:52:15, mattm wrote:
> add:
> ASSERT_EQ(1U, incidents_.size())

Done.

https://codereview.chromium.org/332053005/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/preference_validation_delegate_unittest.cc:228:
EXPECT_EQ(1U, incidents_.size());
On 2014/06/16 19:52:15, mattm wrote:
> Should be ASSERT_EQ (same below)

Done.

[gab, 2014-06-17 14:35:56]

Awesome, lgtm, thanks!

[mattm, 2014-06-17 17:47:24]

lgtm

[grt (UTC plus 2), 2014-06-17 18:05:24]

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2014-06-17 18:08:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/grt@chromium.org/332053005/80001

[commit-bot: I haz the power, 2014-06-17 20:28:10]

Change committed as 277852