elves / elvish

Powerful scripting language & versatile interactive shell
https://elv.sh/
BSD 2-Clause "Simplified" License
5.53k stars 297 forks source link

Improve consistency of the testutil.Set() pattern #1678

Closed krader1961 closed 11 months ago

krader1961 commented 1 year ago

While working on the fix for issue #1674 I noticed that most, but not all, uses of testutil.Set() explicitly modified a pointer to a function being mocked. However, there are a few cases that use a different pattern. This changes those cases so future readers of the code aren't wondering why there are two patterns for using testutil.Set(). As a bonus this allows removing one source file that exists only to enable the alternative pattern being eliminated.

This change also includes a few renaming changes that aren't strictly speaking necessary (e.g., osExit => OSExit) but are included for consistency with similar cases.

xiaq commented 1 year ago

The "dependency variables" shouldn't be exposed as part of the package's API, so they are unexported. It seems that you've changed a lot of them to be exported instead.

Some packages (say pkg) have tests that live in a separate pkg_test package that can't access unexported symbols, and in that case I've used this pattern:

// In code.go, a file that uses a dependency:
package pkg

import "os"

var sleep = os.Sleep

// In testexports_test.go, a test file in the same package:
package pkg

var Sleep = &sleep

// In some_test.go, a test file in the separate pkg_test package:
package pkg_test

import (
  "testing"
  "src.elv.sh/pkg/testutil"
)

func TestFoo(t *testing.T) {
  testutil.Set(Sleep, fakeSleep)
}

I haven't reviewed the entire PR but it seems that at least some of the changes are not respecting this pattern.