Readability review

net/base/prioritized_dispatcher_unittest.cc

 // Copyright (c) 2012 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 <ctype.h>
 #include <string>
 
 #include "base/compiler_specific.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
 #include "net/base/prioritized_dispatcher.h"
 #include "net/base/request_priority.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
 
 namespace {
 
 // We rely on the priority enum values being sequential having starting at 0,
 // and increasing for lower priorities.
 COMPILE_ASSERT(HIGHEST == 0u &&
                LOWEST > HIGHEST &&
                IDLE > LOWEST &&
                NUM_PRIORITIES > IDLE,
                priority_indexes_incompatible);
 
 class PrioritizedDispatcherTest : public testing::Test {
  public:
   typedef PrioritizedDispatcher::Priority Priority;
   // A job that appends |data| to |log_| when started and '.' when finished.

[stevez, 2012/04/11 16:13:58]

So that means it's ambiguous in the log which job is finishing.  You might
consider having it append +data -data on start/finish instead.

[szym, 2012/05/14 22:56:02]

The intention here is the the order of jobs finishing is not relevant for the
correctness of PrioritizedDispatcher. I've updated EnforceLimits to make this
point.

I've initially written +/- and Aa, but found it overall less readable.

   // This is intended to confirm the execution order of a sequence of jobs added
   // to the dispatcher.
   class TestJob : public PrioritizedDispatcher::Job {
    public:
     TestJob(PrioritizedDispatcherTest* test, char data, Priority priority)

[stevez, 2012/04/11 16:13:58]

Fix parameter order.

[szym, 2012/05/14 22:56:02]

As before, I prefer to leave this one as is. This is a very common pattern in
Chromium codebase: http://goo.gl/4lnej 

Even though the pointer is mutable, we don't consider it "output".

         : test_(test), data_(data), priority_(priority), running_(false) {}
 
     // MSVS does not accept EXPECT_EQ(this, ...) so wrap it up.

[stevez, 2012/04/11 16:13:58]

This doesn't seem so useful to me.  If any expectation involving this actually
triggered, how would you debug the problem?  (it would take some effort just to
understand what was broken, as it's just printing out memory addresses?  unless
there's some magic in EXPECT_EQ to do better, though I don't really see how
there could be).

I'd suggest instead just adding an accessor for data_, and comparing those when
needed.  Then your error messages will be a lot more understandable when they
fire.

[szym, 2012/05/14 22:56:02]

Done.

     PrioritizedDispatcher::Job* this_job() {
       return this;
     }
 
     void Add() {
       EXPECT_TRUE(handle_.is_null());

[stevez, 2012/04/11 16:13:58]

Many of these expectations do not depend on the code under test, they would only
fire if the code of the test itself is incorrect.  For these cases, use CHECK
rather than EXPECT.  Only use EXPECT/ASSERT for incorrect behavior of the code
you're actually testing.

[szym, 2012/05/14 22:56:02]

I generally agree. However, a PrioritizedDispatcher which starts the jobs in
faulty order or incorrectly returns null handle will lead to crash in subsequent
calls to ChangePriority and Cancel. To make DCHECK safe, we'd need an ASSERT in
the test case body.

[stevez, 2012/05/17 17:57:07]

I was suggesting CHECK for a reason, not DCHECK.  (really, I can't think of a
reason to use DCHECK in test code, you can always crash in test code!).

If you're concerned about a null handle being returned, ASSERT appropriately
when you receive it from the dispatcher.

[szym, 2012/05/17 19:52:01]

An ASSERT_ within TestJob::Add or TestJob::ChangePriority will not quit the
test. I added ASSERTs to test body.

       EXPECT_FALSE(running_);
       size_t num_queued = dispatch().num_queued_jobs();
       size_t num_running = dispatch().num_running_jobs();
 
       handle_ = dispatch().Add(this, priority_);
 
       if (handle_.is_null()) {
         EXPECT_EQ(num_queued, dispatch().num_queued_jobs());
         EXPECT_TRUE(running_);
         EXPECT_EQ(num_running + 1, dispatch().num_running_jobs());
@@ skipping 49 matching lines @@
     virtual void Start() OVERRIDE {
       EXPECT_FALSE(running_);
       handle_ = PrioritizedDispatcher::Handle();
       running_ = true;
       test_->log_.append(1u, data_);
     }
 
    private:
     PrioritizedDispatcher& dispatch() { return *(test_->dispatch_); }
 
     PrioritizedDispatcherTest* test_;

[stevez, 2012/04/11 16:13:58]

This seems a bit sketchy to me (it sets up circular dependencies among these
classes).  Why not just pass in pointers to the log and dispatcher; or a pointer
to some struct containing them, which the test then owns.

[szym, 2012/05/14 22:56:02]

Done.

 
     char data_;
     Priority priority_;
 
     PrioritizedDispatcher::Handle handle_;
     bool running_;
   };
 
  protected:
   void Prepare(const PrioritizedDispatcher::Limits& limits) {
     dispatch_.reset(new PrioritizedDispatcher(limits));
   }
 
   TestJob* AddJob(char data, Priority priority) {
     TestJob* job = new TestJob(this, data, priority);
     jobs_.push_back(job);
     job->Add();
     return job;
   }
 
   void Expect(std::string log) {
     EXPECT_EQ(0u, dispatch_->num_queued_jobs());
     EXPECT_EQ(0u, dispatch_->num_running_jobs());
     EXPECT_EQ(log, log_);
     log_.clear();
   }
 
   std::string log_;
   scoped_ptr<PrioritizedDispatcher> dispatch_;

[stevez, 2012/04/11 16:13:58]

dispatcher_

[szym, 2012/05/14 22:56:02]

Done.

   ScopedVector<TestJob> jobs_;
 };
 
 TEST_F(PrioritizedDispatcherTest, AddAFIFO) {
   // Allow only one running job.
   PrioritizedDispatcher::Limits limits(NUM_PRIORITIES, 1);
   Prepare(limits);
 
   TestJob* job_a = AddJob('a', IDLE);
   TestJob* job_b = AddJob('b', IDLE);
@@ skipping 111 matching lines @@
 
   EXPECT_EQ(job_b, dispatch_->EvictOldestLowest());
   EXPECT_EQ(job_d, dispatch_->EvictOldestLowest());
 
   job_a->Finish();
   job_c->Finish();
   job_e->Finish();
 
   Expect("a.c.e.");
 }
 

[stevez, 2012/04/11 16:13:58]

What about cancelling a null, or not-in-queue handle (eg handle from a different
queue).

What about evicting when the queue is empty?

[szym, 2012/05/14 22:56:02]

Not legal. Will crash on DCHECK.
Added.

[stevez, 2012/05/17 17:57:07]

Great, why not add a death test for that?

[szym, 2012/05/17 19:52:01]

I can't find an example of a death test in Chromium. From sheriffing experience,
tests which fail with CHECKs or DCHECKs typically end up disabled.

[szym, 2012/05/17 20:28:58]

Forgive my ignorance. I now added a death test.

 }  // namespace
 
 }  // namespace net
 
+