From 6723b9a2264bf42d41d286723b33bce0b8450677 Mon Sep 17 00:00:00 2001 From: Qi Xiao Date: Sun, 19 Jun 2022 23:56:18 +0100 Subject: [PATCH] Use consistent pattern for mutating variables in tests. - Use testutil.Set. - Only export such variables to tests. --- pkg/edit/highlight/highlight.go | 7 +++---- pkg/edit/highlight/highlighter_test.go | 8 ++++---- pkg/eval/builtin_fn_misc.go | 14 ++++---------- pkg/eval/builtin_fn_misc_test.go | 12 ++++++------ pkg/eval/builtin_fn_misc_unix_test.go | 25 +++++++------------------ pkg/eval/pwd.go | 6 +++--- pkg/eval/pwd_test.go | 12 ++---------- pkg/eval/testexport_test.go | 7 ++++++- pkg/fsutil/gethome.go | 5 +---- 9 files changed, 36 insertions(+), 60 deletions(-) diff --git a/pkg/edit/highlight/highlight.go b/pkg/edit/highlight/highlight.go index 7fdb0d3d..7851ec3a 100644 --- a/pkg/edit/highlight/highlight.go +++ b/pkg/edit/highlight/highlight.go @@ -22,9 +22,8 @@ type cmdRegion struct { cmd string } -// MaxBlockForLate specifies the maximum wait time to block for late results. -// It can be changed for test cases. -var MaxBlockForLate = 10 * time.Millisecond +// Maximum wait time to block for late results. Can be changed for test cases. +var maxBlockForLate = 10 * time.Millisecond // Highlights a piece of Elvish code. func highlight(code string, cfg Config, lateCb func(ui.Text)) (ui.Text, []error) { @@ -117,7 +116,7 @@ func highlight(code string, cfg Config, lateCb func(ui.Text)) (ui.Text, []error) select { case late := <-lateCh: return late, errors - case <-time.After(MaxBlockForLate): + case <-time.After(maxBlockForLate): go func() { lateCb(<-lateCh) }() diff --git a/pkg/edit/highlight/highlighter_test.go b/pkg/edit/highlight/highlighter_test.go index 029cdd4e..bf661dbf 100644 --- a/pkg/edit/highlight/highlighter_test.go +++ b/pkg/edit/highlight/highlighter_test.go @@ -24,7 +24,7 @@ var styles = ui.RuneStylesheet{ func TestHighlighter_HighlightRegions(t *testing.T) { // Force commands to be delivered synchronously. - MaxBlockForLate = testutil.Scaled(100 * time.Millisecond) + testutil.Set(t, &maxBlockForLate, testutil.Scaled(100*time.Millisecond)) hl := NewHighlighter(Config{ HasCommand: func(name string) bool { return name == "ls" }, }) @@ -127,7 +127,7 @@ func testThat(t *testing.T, hl *Highlighter, c c) { func TestHighlighter_HasCommand_LateResult_Async(t *testing.T) { // When the HasCommand callback takes longer than maxBlockForLate, late // results are delivered asynchronously. - MaxBlockForLate = testutil.Scaled(time.Millisecond) + testutil.Set(t, &maxBlockForLate, testutil.Scaled(time.Millisecond)) hl := NewHighlighter(Config{ // HasCommand is slow and only recognizes "ls". HasCommand: func(cmd string) bool { @@ -150,7 +150,7 @@ func TestHighlighter_HasCommand_LateResult_Async(t *testing.T) { func TestHighlighter_HasCommand_LateResult_Sync(t *testing.T) { // When the HasCommand callback takes shorter than maxBlockForLate, late // results are delivered asynchronously. - MaxBlockForLate = testutil.Scaled(100 * time.Millisecond) + testutil.Set(t, &maxBlockForLate, testutil.Scaled(100*time.Millisecond)) hl := NewHighlighter(Config{ // HasCommand is fast and only recognizes "ls". HasCommand: func(cmd string) bool { @@ -175,7 +175,7 @@ func TestHighlighter_HasCommand_LateResultOutOfOrder(t *testing.T) { // "ls" and is dropped. // Make sure that the HasCommand callback takes longer than maxBlockForLate. - MaxBlockForLate = testutil.Scaled(time.Millisecond) + testutil.Set(t, &maxBlockForLate, testutil.Scaled(time.Millisecond)) hlSecond := make(chan struct{}) hl := NewHighlighter(Config{ diff --git a/pkg/eval/builtin_fn_misc.go b/pkg/eval/builtin_fn_misc.go index e53f4c61..9257ef9c 100644 --- a/pkg/eval/builtin_fn_misc.go +++ b/pkg/eval/builtin_fn_misc.go @@ -409,15 +409,9 @@ func deprecate(fm *Frame, msg string) { fm.Deprecate(msg, ctx, 0) } -// TimeAfter is used by the sleep command to obtain a channel that is delivered -// a value after the specified time. -// -// It is a variable to allow for unit tests to efficiently test the behavior of -// the `sleep` command, both by eliminating an actual sleep and verifying the -// duration was properly parsed. -var TimeAfter = func(fm *Frame, d time.Duration) <-chan time.Time { - return time.After(d) -} +// Reference to time.After, can be mutated for testing. Takes an additional +// Frame argument to allow inspection of the value of d in tests. +var timeAfter = func(fm *Frame, d time.Duration) <-chan time.Time { return time.After(d) } //elvdoc:fn sleep // @@ -486,7 +480,7 @@ func sleep(fm *Frame, duration any) error { select { case <-fm.Interrupts(): return ErrInterrupted - case <-TimeAfter(fm, d): + case <-timeAfter(fm, d): return nil } } diff --git a/pkg/eval/builtin_fn_misc_test.go b/pkg/eval/builtin_fn_misc_test.go index 2ea2c2e0..14177e8f 100644 --- a/pkg/eval/builtin_fn_misc_test.go +++ b/pkg/eval/builtin_fn_misc_test.go @@ -108,13 +108,13 @@ func TestUseMod(t *testing.T) { ) } -func timeAfterMock(fm *Frame, d time.Duration) <-chan time.Time { - fm.ValueOutput().Put(d) // report to the test framework the duration we received - return time.After(0) -} - func TestSleep(t *testing.T) { - TimeAfter = timeAfterMock + testutil.Set(t, TimeAfter, + func(fm *Frame, d time.Duration) <-chan time.Time { + fm.ValueOutput().Put(d) + return time.After(0) + }) + Test(t, That(`sleep 0`).Puts(0*time.Second), That(`sleep 1`).Puts(1*time.Second), diff --git a/pkg/eval/builtin_fn_misc_unix_test.go b/pkg/eval/builtin_fn_misc_unix_test.go index 7241852e..5580032c 100644 --- a/pkg/eval/builtin_fn_misc_unix_test.go +++ b/pkg/eval/builtin_fn_misc_unix_test.go @@ -7,32 +7,21 @@ import ( "testing" "time" + "golang.org/x/sys/unix" . "src.elv.sh/pkg/eval" "src.elv.sh/pkg/testutil" . "src.elv.sh/pkg/eval/evaltest" ) -func interruptedTimeAfterMock(fm *Frame, d time.Duration) <-chan time.Time { - if d == time.Second { - // Special-case intended to verity that a sleep can be interrupted. - go func() { - // Wait a little bit to ensure that the control flow in the "sleep" - // function is in the select block when the interrupt is sent. - time.Sleep(testutil.Scaled(time.Millisecond)) - p, _ := os.FindProcess(os.Getpid()) - p.Signal(os.Interrupt) - }() - return time.After(1 * time.Second) - } - panic("unreachable") -} +func TestSleep_Interrupt(t *testing.T) { + testutil.Set(t, TimeAfter, + func(fm *Frame, d time.Duration) <-chan time.Time { + go unix.Kill(os.Getpid(), unix.SIGINT) + return time.After(d) + }) -func TestInterruptedSleep(t *testing.T) { - TimeAfter = interruptedTimeAfterMock Test(t, - // Special-case that should result in the sleep being interrupted. See - // timeAfterMock above. That(`sleep 1s`).Throws(ErrInterrupted, "sleep 1s"), ) } diff --git a/pkg/eval/pwd.go b/pkg/eval/pwd.go index 734cb994..172a60d6 100644 --- a/pkg/eval/pwd.go +++ b/pkg/eval/pwd.go @@ -18,13 +18,13 @@ type pwdVar struct { var _ vars.Var = pwdVar{} -// Getwd allows for unit test error injection. -var Getwd func() (string, error) = os.Getwd +// Can be mutated in tests. +var getwd func() (string, error) = os.Getwd // Get returns the current working directory. It returns "/unknown/pwd" when // it cannot be determined. func (pwdVar) Get() any { - pwd, err := Getwd() + pwd, err := getwd() if err != nil { // This should really use the path separator appropriate for the // platform but in practice this hardcoded string works fine. Both diff --git a/pkg/eval/pwd_test.go b/pkg/eval/pwd_test.go index 7f039abc..9fafb6d5 100644 --- a/pkg/eval/pwd_test.go +++ b/pkg/eval/pwd_test.go @@ -52,15 +52,7 @@ func TestBuiltinPwd(t *testing.T) { // Verify the behavior when the CWD cannot be determined. func TestBuiltinPwd_GetwdError(t *testing.T) { - origGetwd := Getwd - Getwd = mockGetwdWithError - defer func() { Getwd = origGetwd }() + testutil.Set(t, Getwd, func() (string, error) { return "", errors.New("cwd unknown") }) - Test(t, - That(`put $pwd`).Puts("/unknown/pwd"), - ) -} - -func mockGetwdWithError() (string, error) { - return "", errors.New("cwd unknown") + Test(t, That(`put $pwd`).Puts("/unknown/pwd")) } diff --git a/pkg/eval/testexport_test.go b/pkg/eval/testexport_test.go index a5ea3675..054aee30 100644 --- a/pkg/eval/testexport_test.go +++ b/pkg/eval/testexport_test.go @@ -1,3 +1,8 @@ package eval -var GetHome = &getHome +// Pointers to functions that can be mutated for testing. +var ( + GetHome = &getHome + Getwd = &getwd + TimeAfter = &timeAfter +) diff --git a/pkg/fsutil/gethome.go b/pkg/fsutil/gethome.go index d5b323e7..49a09484 100644 --- a/pkg/fsutil/gethome.go +++ b/pkg/fsutil/gethome.go @@ -9,9 +9,6 @@ import ( "src.elv.sh/pkg/env" ) -// CurrentUser allows for unit test error injection. -var CurrentUser func() (*user.User, error) = user.Current - // GetHome finds the home directory of a specified user. When given an empty // string, it finds the home directory of the current user. func GetHome(uname string) (string, error) { @@ -28,7 +25,7 @@ func GetHome(uname string) (string, error) { var u *user.User var err error if uname == "" { - u, err = CurrentUser() + u, err = user.Current() } else { u, err = user.Lookup(uname) }