Remove broken variables from sync's AllStatus

chrome/browser/sync/engine/syncer_unittest.cc

 // Copyright (c) 2011 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.
 //
 // Syncer unit tests. Unfortunately a lot of these tests
 // are outdated and need to be reworked and updated.
 
 #include <algorithm>
 #include <limits>
 #include <list>
@@ skipping 1681 matching lines @@
   mock_server_->AddUpdateDirectory(1, 0, "", 1, 10);
   // And one legal one that we're going to delete.
   mock_server_->AddUpdateDirectory(2, 0, "FOO", 1, 10);
   SyncShareAsDelegate();
   // Delete the legal one. The new update has a null name.
   mock_server_->AddUpdateDirectory(2, 0, "", 2, 20);
   mock_server_->SetLastUpdateDeleted();
   SyncShareAsDelegate();
 }
 
-TEST_F(SyncerTest, DontGetStuckWithTwoSameNames) {

[tim (not reviewing), 2012/01/10 17:01:47]

Is it bad to test that two updates with the same name doesn't hose sync? Can we
retrofit this test to make it useful?

[rlarocque, 2012/01/10 21:39:32]

My understanding is that we don't actually look at the names of items a whole
lot, even in bookrmarks code.  Also keep in mind this test is configured with
"passive" workers, so no data type specific code is being run.

This test also makes assumptions about the way SyncShare is invoked, which I
think is a mistake.  We've seen that the HasMoreToSync() method and
SyncScheduler's loop to run multiple sync cycles has a huge impact on system
behaviour.  This test bypasses that code.

I'm not sure there's a lot of value in this test if we look at it from a "white
box" perspective.  Could it be made useful?  Maybe.  Is it worth the effort? 
Probably not.

I suspect that most of the tests in this file are not very useful.  But we
haven't had any reason to delete them yet, so they live on.

[tim (not reviewing), 2012/01/11 02:15:35]

That's not true, actually. We haven't deleted them because they are the only
tests that actually exercise the syncer commands together :). I think there is
use to that, whether or not they represent a complete integration test of
exactly the behavior in practice. I think the assumptions being made or hacks
around SyncShare are there because that code isn't the target of these tests.
The target is really various bits of code and branches in syncer commands and
syncer_util that maintain generic integrity of the full tree of data and not the
mechanics of running a session. Incidentally, it wasn't ever the intent of
having data-type specific tests here since we have yet to implement data type
specific conflict resolution at this layer (though implicitly a lot of the cases
here may be biased towards bookmarks due to hierarchical relationships).

If we find otherwise, and that no test in here is currently effectively testing
what it once was (or those things are all irrelevant now), then it should become
our top priority to test the code we have. There are things tested in here that
can no longer happen in practice, but I doubt that is the case for every single
test.

The main argument I would make as to why it is worth having a test covering what
this was intending to cover is because the code is still there.  If we rip out
all code related to names, then we should rip out the test.  But if we have code
that we just stop testing and leave to fester, all of a sudden that path that
you didn't care about can spiral out of control and cause harm.

I wont absolutely insist, but is it really that hard to keep the test working?
Could we not check that things don't wind up in conflict? That a subsequent
SyncShare works as expected?

[rlarocque, 2012/01/11 19:58:36]

I exaggerated when I commented on the state of this file.  But I really do think
that this particular test serves no purpose.  

As far as I can tell, this test is trying to test the case where two new entries
have the same NON_UNIQUE_NAME and different IDs.  

I grep'ed for NON_UNIQUE_NAME and found that it plays a minor role in conflict
resolution, which won't be executed in this case because these two entries do
not conflict.  All other references to NON_UNIQUE_NAME seem to be in simple
getter/setter methods, functions that copy server fields to client fields and
vice-versa, etc.  I don't see anywhere in the syncer code where we compare the
(SERVER_)NON_UNIQUE_NAMEs of two different entries.

If that's the case, any brokenness that could be detected by this test would
also be detected by any other test that grabs a two sibling updates from the
server and applies them by manually calling SyncShare.

To answer your question, no, it's not hard to keep this test working.  I just
don't see any reason why we should.

See also: crbug.com/107816, which explains why syncer_stuck was always false in
practice and which makes obsolete the current definition of
"SyncRepeatedlyToTriggerStuckSignal()".

-  // We should not get stuck here because we get
-  // two server updates with exactly the same name.
-  ScopedDirLookup dir(syncdb_.manager(), syncdb_.name());
-  ASSERT_TRUE(dir.good());
-  mock_server_->AddUpdateDirectory(1, 0, "foo:", 1, 10);
-  SyncShareAsDelegate();
-  mock_server_->AddUpdateDirectory(2, 0, "foo:", 1, 20);
-  SyncRepeatedlyToTriggerStuckSignal(session_.get());
-  EXPECT_FALSE(session_->status_controller().syncer_status().syncer_stuck);
-  saw_syncer_event_ = false;
-}
-
 TEST_F(SyncerTest, TestBasicUpdate) {
   ScopedDirLookup dir(syncdb_.manager(), syncdb_.name());
   ASSERT_TRUE(dir.good());
   string id = "some_id";
   string parent_id = "0";
   string name = "in_root";
   int64 version = 10;
   int64 timestamp = 10;
   mock_server_->AddUpdateDirectory(id, parent_id, name, version, timestamp);
 
@@ skipping 1522 matching lines @@
   SyncShareAsDelegate();
   {
     WriteTransaction trans(FROM_HERE, UNITTEST, dir);
     MutableEntry entry(&trans, CREATE, trans.root_id(), "Copy of base");
     WriteTestDataToEntry(&trans, &entry);
   }
   mock_server_->AddUpdateBookmark(1, 0, "Copy of base", 50, 50);
   SyncRepeatedlyToTriggerConflictResolution(session_.get());
 }
 
-TEST_F(SyncerTest, OneBajillionUpdates) {
-  ScopedDirLookup dir(syncdb_.manager(), syncdb_.name());
-  CHECK(dir.good());
-  int one_bajillion = 4000;
-
-  syncable::Id parent_id = ids_.MakeServer("Parent");
-  mock_server_->AddUpdateDirectory(parent_id, ids_.root(), "foo", 1, 1);
-
-  for (int i = 1; i <= one_bajillion; ++i) {
-    syncable::Id item_id = ids_.FromNumber(i);
-    mock_server_->AddUpdateDirectory(item_id, parent_id, "dude", 1, 1);
-  }
-
-  syncer_->SyncShare(session_.get(), SYNCER_BEGIN, SYNCER_END);
-  EXPECT_FALSE(session_->status_controller().syncer_status().syncer_stuck);

[tim (not reviewing), 2012/01/10 17:01:47]

I kind of have the same question as above.  I don't think the spirit of this
test is obsolete.

[rlarocque, 2012/01/10 21:39:32]

I'm not sure this ever made sense.  What did we hope to prove with 4000 nested
directories that can't be proven with 10?  Why was there a risk that the syncer
could get stuck in this case, event when syncer_stuck actually meant something?

[tim (not reviewing), 2012/01/11 02:15:35]

Well, even though I closed the bug myself with a similar argument, look at bug
20185. We have people with ~1000 bookmarks, so I don't think it is ridiculous to
have a test that would test *that* (4k may be excessive) and doesn't either run
out of memory or time out or fail for some strange batching bug or who knows
what.

[rlarocque, 2012/01/11 19:58:36]

I agree, scale* tests are useful.  But a scale test does not belong among our
unit tests.  Unit tests should aim for good coverage of small areas of code and
fast execution time.

A scale test written in this way is likely to be ineffective.  What if the scale
issue is located in the model association code?  An integration test would be
much more likely to uncover scale issues.  Even better would be to rewrite this
as a performance test, which would make it easier to track performance
regressions in addition to verifying that this scenario does not trigger a
crash.

* scale, in this case, refers scale on the client side as opposed to our usual
definition.

-}
-
 // In this test a long changelog contains a child at the start of the changelog
 // and a parent at the end. While these updates are in progress the client would
 // appear stuck.
 TEST_F(SyncerTest, LongChangelistWithApplicationConflict) {
   ScopedDirLookup dir(syncdb_.manager(), syncdb_.name());
   CHECK(dir.good());
   const int depth = 400;
   syncable::Id folder_id = ids_.FromNumber(1);
 
   // First we an item in a folder in the root. However the folder won't come
   // till much later.
   syncable::Id stuck_entry_id = TestIdFactory::FromNumber(99999);
   mock_server_->AddUpdateDirectory(stuck_entry_id,
       folder_id, "stuck", 1, 1);
   mock_server_->SetChangesRemaining(depth - 1);
   syncer_->SyncShare(session_.get(), SYNCER_BEGIN, SYNCER_END);
 
   // Buffer up a very long series of downloads.
   // We should never be stuck (conflict resolution shouldn't
   // kick in so long as we're making forward progress).
   for (int i = 0; i < depth; i++) {
     mock_server_->NextUpdateBatch();
     mock_server_->SetNewTimestamp(i + 1);
     mock_server_->SetChangesRemaining(depth - i);
   }
 
   syncer_->SyncShare(session_.get(), SYNCER_BEGIN, SYNCER_END);
-  EXPECT_FALSE(session_->status_controller().syncer_status().syncer_stuck);
 
   // Ensure our folder hasn't somehow applied.
   {
     ReadTransaction trans(FROM_HERE, dir);
     Entry child(&trans, GET_BY_ID, stuck_entry_id);
     EXPECT_TRUE(child.good());
     EXPECT_TRUE(child.Get(IS_UNAPPLIED_UPDATE));
     EXPECT_TRUE(child.Get(IS_DEL));
     EXPECT_FALSE(child.Get(IS_UNSYNCED));
   }
@@ skipping 1538 matching lines @@
   Add(low_id_);
   Add(high_id_);
   SyncShareAsDelegate();
   ExpectLocalOrderIsByServerId();
 }
 
 const SyncerTest::CommitOrderingTest
 SyncerTest::CommitOrderingTest::LAST_COMMIT_ITEM = {-1, TestIdFactory::root()};
 
 }  // namespace browser_sync