rAc - Only show countries we're able to fill in.

chrome/browser/ui/autofill/country_combobox_model_unittest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/autofill/country_combobox_model.h"
 
 #include "base/memory/scoped_ptr.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/autofill/core/browser/autofill_country.h"
 #include "components/autofill/core/browser/test_personal_data_manager.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_ui.h"
 #include "third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_ui_component.h"
 
 namespace autofill {
 
 class CountryComboboxModelTest : public testing::Test {
  public:
   CountryComboboxModelTest() {
     manager_.Init(NULL, profile_.GetPrefs(), false);
     manager_.set_timezone_country_code("KR");
     CreateModel(true);
   }
   virtual ~CountryComboboxModelTest() {}
 
  protected:
   void CreateModel(bool show_partially_supported_countries) {
     model_.reset(new CountryComboboxModel(
-        manager_, show_partially_supported_countries));
+        manager_,
+        std::set<base::string16>(),
+        show_partially_supported_countries));
   }
 
   TestPersonalDataManager* manager() { return &manager_; }
   CountryComboboxModel* model() { return model_.get(); }
 
  private:
   // NB: order is important here - |profile_| must go down after |manager_|.
   TestingProfile profile_;
   TestPersonalDataManager manager_;
   scoped_ptr<CountryComboboxModel> model_;
@@ skipping 27 matching lines @@
   for (int i = 0; i < model()->GetItemCount(); ++i) {
     ASSERT_FALSE(model()->IsItemSeparatorAt(i));
     std::string country_code = model()->countries()[i]->country_code();
     std::vector< ::i18n::addressinput::AddressUiComponent> components =
         ::i18n::addressinput::BuildComponents(country_code);
     ASSERT_FALSE(components.empty());
     for (size_t j = 0; j < components.size(); ++j) {
       EXPECT_NE(components[j].field, ::i18n::addressinput::DEPENDENT_LOCALITY);
     }
   }
 }

[Dan Beam, 2014/03/01 03:38:10]

can you add a test of the country filtering logic here?

[Evan Stade, 2014/03/03 18:37:00]

Adding a test here or in formstructure would not add any more coverage than the
test I have already added.

[Dan Beam, 2014/03/03 22:20:00]

fine

[Ilya Sherman, 2014/03/03 23:55:54]

IMO that means you're testing the wrong things at the wrong levels.  Unit tests
should test a class's public API.  If I make changes to foo.cc and
foo_unittests.cc pass, then I should feel pretty confident that the changes to
foo.cc were correct.  Longer-distance dependencies create confusion and make it
less likely that e.g. tests will be updated as the implementation evolves.

[Evan Stade, 2014/03/04 00:11:30]

This is what the bots are for. They make sure to catch all relevant failures.

If I move the tests to this level, I will still have to leave the tests that are
at the AutofillDialogController unit test level. Thus I will be adding more
tests for no greater coverage. Adding tests here will (a) take time today (b)
take time tomorrow to update when I want to change something (c) test
implementation rather than behavior (all we really care about is that the dialog
itself works, not the fact it uses CountryComboboxModel to work).

[Ilya Sherman, 2014/03/04 00:53:57]

That's true, though bots are much slower than running the tests locally.
The logical extreme of your argument seems to me to be something like "We really
only care whether features work, so there's no point to testing intermediate
classes, as long as their code paths are tested at the feature level."  I
suspect you don't really think that, but I honestly don't see much difference
between that stance and the narrower one that you're adopting here.  IMO "tests
should test a class's full public API" is a simpler rule to follow that results
in better test coverage overall.

Replying to your objections specifically:
  (a) Yes, that's true.  Tests are never free; but IMO it's worth taking the
time to write them.
  (b) This might be true.  On the other hand, it might save time tomorrow for a
developer who hits a clearer test failure.  Again; tests aren't expected to have
zero maintenance cost.  In this case, I'd expect the maintenance cost of a test
for this class to be quite low over the lifetime of the test.
  (c) I'm asking you to test the *behavior* of the CountryComboboxModel class,
which is what is exposed to clients of the class.  This has nothing to do with
decisions that are private to the implementation of the class.

[Evan Stade, 2014/03/04 01:05:14]

In fact I do think this, but unit tests are frequently the most expedient way to
get complete code coverage.
It is a simple rule, but it's not a rule that we follow in Chrome, and it's not
one that I would espouse given the extreme cost of doing this.
when there's value to them, I agree with you.
If you can think of a way to change CountryComboboxModel that will not cause the
test that is already written to fail, and which could not be covered by
improving the test that's already written, then I'll agree CountryComboboxModel
needs more testing.

 
 }  // namespace autofill