3

While writing unit test for a method in go, I am stumped at a problem. First, code snippet under test:

func MehodToBeTested(e Entity) {
  go saveAudit(e)

 //do something on which assertions can be done
}

Entity can be mocked. in the saveAudit method, Entity.Save method is called. In my UT, i want to assert that Entity.Save method is called once. Following is my current UT :

func TestMethod(t *testing.T) {
  var mock = &mockEntity{}
  mock.On("Save").Return(nil)

  //make call to func under test
  MethodToBeTested(mock)

  // Assert that Save is called on Entity
  mock.AssertNumberOfCalls(t, "Save",1)
}

This is giving error saying : Expected number of calls (1) does not match the actual number of calls (0) since the actual call is happening in another go routine. How can i test this?

Jonathan Hall
  • 75,165
  • 16
  • 143
  • 189
zaRRoc
  • 345
  • 1
  • 7
  • 18

3 Answers3

3

I use the same technique. Just wait for goroutine end. Very likely it’s not yet set.

Also, I recommend to run such tests with the race-condition detector. It helps to catch such situations. Then you can add some synchronization to the tests to make them reliable.

Example from my test. A tested function should check in parallel that both web pages contain the specified string. So test should check that tested function has visited both pages

UPDATE: incorrect test was attached. Fixed.

func TestCheckSites_TwoSlowHandlers_BothContain(t *testing.T) {
    var config = GetConfig()
    var v1, v2 bool
    var wg sync.WaitGroup
    wg.Add(2)
    handler1 := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        defer wg.Done()
        v1 = true
        time.Sleep(2 * config.Http.Timeout) // Use double HTTP_TIMEOUT
        io.WriteString(w, "Present")
    })
    ts1 := httptest.NewServer(handler1)
    defer ts1.Close()

    handler2 := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        defer wg.Done()
        v2 = true
        time.Sleep(2 * config.Http.Timeout)
        io.WriteString(w, "Present")
    })
    ts2 := httptest.NewServer(handler2)
    defer ts2.Close()

    result, err := checkSites([]string{ts1.URL, ts2.URL}, "Present")
    assert.Equal(t, nil, err, "Error should be nil")
    assert.Contains(t, []string{""}, result, "Should be empty string")
    //assert.(t, ts1.URL, result, "Should first or second empty string")
    wg.Wait()
    assert.Equal(t, true, v1, "First server should be visited")
    assert.Equal(t, true, v2, "Second server should be visited")
}
Sourabh Upadhyay
  • 1,034
  • 11
  • 31
Eugene Lisitsky
  • 12,113
  • 5
  • 38
  • 59
  • You have a data race over `v1` and `v2`: the variables are updated in separate goroutines which run the handlers of the test server instances and then are read in another goroutine. The fact the race detector does not catch this case is because the `Close()` method on an instance created by `httptest.NewServer()` happens to perform a synchronization, and the race detector only cares about *any* synchronization happening between consequtive read/write or write/read accesses to the same variable done from separate goroutines. – kostix Nov 24 '17 at 07:59
  • A (semantically) correct way to structure a test like this, is to create a channel of capacity 2 (since you have two servers) and have them write to that channel instead of updating variables. After the requests to these servers are complete, you have to drain the channel and inspect the results. Sure, you can do it with two channels or structure your code some other way (such as using a mutex). – kostix Nov 24 '17 at 08:02
  • 1
    @kostix thank you for the attention! Sorry I’ve attached another test where race condition ’s not important. Please find the correct one. Main difference: it uses `WaitGroup` to wait until both functions are finished. – Eugene Lisitsky Nov 24 '17 at 08:10
0

First, what you're doing isn't what I would consider true unit testing, since you're testing multiple units at once. For "true" unit testing, test each function separately:

func TestMethodToBeTested(t *testing.T) {
    // Test the main function

func TestAuditSave(t *testing.T) {
    // Test the code executed in the goroutine

With this separation of concerns, all that's left to do is not (meaningfully) execute the goroutine when executing TestMethodToBeTested. This could be done any number of ways:

  1. If saveAudit's behavior can be ignored, just ignore it--but also don't test for it.
  2. It could be moved into an interface, or other variable, so that a stub can be put in its place. Example:

    func (x *X) MethodToBeTested(e Entity) {
        go x.saveAudit(e)
        // more code
    }
    

    In this way, you can substitute a dummy saveAudit method in your tests.

The latter is the approach I would generally recommend, even when using non-go-routines, as it makes it easy to test each component individually (i.e. what I call "true" unit testing).

Jonathan Hall
  • 75,165
  • 16
  • 143
  • 189
  • If a requirement of the unit under test is "saves audit", even if that is done by a function in a separate goroutine, then it cannot be ignored, right? Do you have a recommendation for for example validating that a value was sent over a channel? This sounds quite hard to mock to me. – minitauros Oct 20 '18 at 11:06
  • @minitauros: Your "requirement" matches my explanation in the opening paragraph: It's not a _unit_ test, it's an integration test. Test your "audit" function (the one you're running in a goroutine) separately from your function that calls it. This is the only way to do true _unit_ testing. – Jonathan Hall Oct 21 '18 at 13:42
-1

@Flimzy Writing this here because a comment cannot contain nice code examples. Hope that's OK. Consider the following (stupid, but for example's sake):

type MyStruct struct {
    counter int
}

func (s *MyStruct) Add(item string) {
    s.ConfirmAdd()
}

func (s *MyStruct) ConfirmAdd() {
    s.counter++
}

A test for ConfirmAdd() could be as follows

func TestConfirmAdd(t *testing.T) {
    s := MyStruct{}
    s.ConfirmAdd()
    Assert(s.counter, Equals, 1)
}

When writing a test for Add(), would you write nothing? Feels bad not to assert that ConfirmAdd() was called.

minitauros
  • 1,920
  • 18
  • 20