test: fix flaky test

[elhimov]

Helper method that performs initial assigning of master/replica roles and is widely used in ConnectionPool tests was adjusted to wait for the roles to be applied successfully.
Prior to this patch it doesn't, so sometimes subsequent test code might work unexpectedly (the problem was caught with
TestConnectionHandlerOpenUpdateClose)

Related issues:

Closes #452

[bigbes]

this is more readable version of this code, imo:

Suggested change

	// Wait for the role to be applied.
	for {
	time.Sleep(10 * time.Millisecond)
	var reason string
	data, err := conn.Do(tarantool.NewCallRequest("box.info")).Get()
	switch {
	case err != nil:
	reason = fmt.Sprintf("failed to get box.info: %s", err)
	case len(data) < 1:
	reason = "box.info is empty"
	default:
	status, statusFound := data[0].(map[interface{}]interface{})["status"]
	readonly, readonlyFound := data[0].(map[interface{}]interface{})["ro"]
	switch {
	case !statusFound:
	reason = "box.info.status is missing"
	case status != "running":
	reason = fmt.Sprintf("box.info.status='%s' (waiting for 'running')", status)
	case !readonlyFound:
	reason = "box.info.ro is missing"
	case readonly != isReplica:
	reason = fmt.Sprintf("box.info.ro='%v' (waiting for '%v')", readonly, isReplica)
	}
	}
	if len(reason) == 0 {
	break
	}
	select {
	case <-ctx.Done():
	return fmt.Errorf("%w: failed to apply role, the last reason: %s", ctx.Err(), reason)
	default:
	continue
	}
	}
	var reason string
	// Wait for the role to be applied.
	for {
	select {
	case <-time.After(10 * time.Millisecond):
	case <-ctx.Done():
	return fmt.Errorf("%w: failed to apply role, the last reason: %s", ctx.Err(), reason)
	}
	data, err := conn.Do(tarantool.NewCallRequest("box.info")).Get()
	switch {
	case err != nil:
	reason = fmt.Sprintf("failed to get box.info: %s", err)
	continue
	case len(data) < 1:
	reason = "box.info is empty"
	continue
	}
	status, statusFound := data[0].(map[interface{}]interface{})["status"]
	readonly, readonlyFound := data[0].(map[interface{}]interface{})["ro"]
	switch {
	case !statusFound:
	reason = "box.info.status is missing"
	case status != "running":
	reason = fmt.Sprintf("box.info.status='%s' (waiting for 'running')", status)
	case !readonlyFound:
	reason = "box.info.ro is missing"
	case readonly != isReplica:
	reason = fmt.Sprintf("box.info.ro='%v' (waiting for '%v')", readonly, isReplica)
	default:
	return nil
	}
	}

[bigbes]

It can be further optimised for readability with something like that:

func checkInfoStatus(data []interface{}) string {
	status, statusFound := data[0].(map[interface{}]interface{})["status"]
	switch {
	case !statusFound:
		return "box.info.status is missing"
	case status != "running":
		return fmt.Sprintf("box.info.status='%s' (waiting for 'running')", status)
	default:
		return ""
	}
}

func checkInfoRO(data []interface{}, isReplica bool) string {
		readonly, readonlyFound := data[0].(map[interface{}]interface{})["ro"]
	switch {
	case !readonlyFound:
		reason = "box.info.ro is missing"
	case readonly != isReplica:
		reason = fmt.Sprintf("box.info.ro='%v' (waiting for '%v')", readonly, isReplica)
	default:
		return nil
	}
}

func SetInstanceRO(ctx context.Context, dialer tarantool.Dialer, connOpts tarantool.Opts,
	isReplica bool) error {
	conn, err := tarantool.Connect(ctx, dialer, connOpts)
	if err != nil {
		return err
	}

	defer conn.Close()

	req := tarantool.NewCallRequest("box.cfg").
		Args([]interface{}{map[string]bool{"read_only": isReplica}})
	if _, err := conn.Do(req).Get(); err != nil {
		return err
	}

	var reason string

	// Wait for the role to be applied.
	for {
		select {
		case <-time.After(10 * time.Millisecond):
		case <-ctx.Done():
			return fmt.Errorf("%w: failed to apply role, the last reason: %s", ctx.Err(), reason)
		}

		data, err := conn.Do(tarantool.NewCallRequest("box.info")).Get()
		switch {
		case err != nil:
			reason = fmt.Sprintf("failed to get box.info: %s", err)
		case len(data) < 1:
			reason = "box.info is empty"
		case checkInfoStatus(data) != "":
			reason = checkInfoStatus(data)
		case checkInfoRO(data) != "":
			reason = checkInfoRO(data)
		default:
			return nil
		}
	}
}

[elhimov]

Rearranged in a slightly different manner that seems more readable.

[bigbes]

Usually it's better not to use wg.Add() in a cycle, to avoid possible race conditions if starting goroutine and awaiter are different ones.
There's no problem in that code, but rule of thumb is to avoid that kind of problems without double-checking: if we know number of workers before running them - use wg.Add(count).

	errs := make([]error, len(dialers))
	var wg sync.WaitGroup
	wg.Add(len(dialers))
	for i, dialer := range dialers {
		// Pass loop variables to avoid its closure.
		go func(i int, dialer tarantool.Dialer) {
			defer wg.Done()
			errs[i] = SetInstanceRO(ctx, dialer, connOpts, roles[i])
		}(i, dialer)
	}
	wg.Wait()

it's just a nitpicking, but it can save some time in the future.

[elhimov]

Fixed.

[bigbes]

Theoretically, I understand these changes and believe they should work, but I can't guarantee it.
It's up to you to agree or disagree with style comments, since it's test code.

[oleg-jukovec]

It looks better to add a context param to the SetClusterRO call instead of the hidden-confusing-context-timeout.

We could do that since we don't guarantee a backward compatibility for the test_helper package.

func SetClusterRO(dialers []tarantool.Dialer, connOpts tarantool.Opts,
	roles []bool) error {

->

func SetClusterRO(ctx context.Context, dialers []tarantool.Dialer,
	connOpts tarantool.Opts, roles []bool) error {

[elhimov]

I understand that we can modify API here, but it doesn't seem to be the case.
Context is not a kind of global entity, it is always created in some function and then passed into some function that accepts context. Does this make call of such a context-creator-function as hidden-confusing-context? For me context here is just a kind of implementation details. It doesn't seem that caller need to know about it and I see no problem that he doesn't.

[oleg-jukovec]

This is a matter of expected behavior from a call from a code-reader point of view. I insist on this change and the addition of a cotext as a first call argument. Please, add it.

[elhimov]

Done.

[oleg-jukovec]

Please see unresolved comment above.