Skip to content

Draft for discussing testing around extension update checking behavior #9985

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
andyfeller merged 5 commits into from
Dec 9, 2024
+647 −176
Merged

Draft for discussing testing around extension update checking behavior #9985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6bd01d5
Initial extension update check testing
Dec 1, 2024
97630fe
Enhance extension manager and tests
Dec 9, 2024
a55f50b
Remove Extension.FullName() from interface
Dec 9, 2024
a79e086
Test whether extension update should be checked
andyfeller Dec 9, 2024
10f62b1
Expand update tests for local extensions
andyfeller Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions cmd/gen-docs/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,3 +127,7 @@ func (e *em) Create(_ string, _ extensions.ExtTemplateType) error {
}

func (e *em) EnableDryRunMode() {}

func (e *em) UpdateDir(_ string) string {
return ""
}
11 changes: 8 additions & 3 deletions internal/update/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,14 @@ func ShouldCheckForExtensionUpdate() bool {
return !IsCI() && IsTerminal(os.Stdout) && IsTerminal(os.Stderr)
}

func CheckForExtensionUpdate(em extensions.ExtensionManager, ext extensions.Extension, stateFilePath string) (*ReleaseInfo, error) {
func CheckForExtensionUpdate(em extensions.ExtensionManager, ext extensions.Extension, now time.Time) (*ReleaseInfo, error) {
if ext.IsLocal() {
return nil, nil
}

stateFilePath := filepath.Join(em.UpdateDir(ext.Name()), "state.yml")
stateEntry, _ := getStateEntry(stateFilePath)
if stateEntry != nil && time.Since(stateEntry.CheckedForUpdateAt).Hours() < 24 {
if stateEntry != nil && now.Sub(stateEntry.CheckedForUpdateAt).Hours() < 24 {
return nil, nil
}

Expand All @@ -54,7 +59,7 @@ func CheckForExtensionUpdate(em extensions.ExtensionManager, ext extensions.Exte
URL: ext.URL(),
}

err := setStateEntry(stateFilePath, time.Now(), *releaseInfo)
err := setStateEntry(stateFilePath, now, *releaseInfo)
if err != nil {
return nil, err
}
Expand Down
308 changes: 308 additions & 0 deletions internal/update/update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,14 @@ import (
"log"
"net/http"
"os"
"path/filepath"
"testing"
"time"

"github.com/cli/cli/v2/pkg/cmd/extension"
"github.com/cli/cli/v2/pkg/extensions"
"github.com/cli/cli/v2/pkg/httpmock"
"github.com/stretchr/testify/require"
)

func TestCheckForUpdate(t *testing.T) {
Expand Down Expand Up @@ -117,6 +122,309 @@ func TestCheckForUpdate(t *testing.T) {
}
}

func TestCheckForExtensionUpdate(t *testing.T) {
now := time.Date(2024, 12, 17, 12, 0, 0, 0, time.UTC)
previousTooSoon := now.Add(-23 * time.Hour).Add(-59 * time.Minute).Add(-59 * time.Second)
previousOldEnough := now.Add(-24 * time.Hour)

tests := []struct {
name string
extCurrentVersion string
extLatestVersion string
extKind extension.ExtensionKind
extURL string
previousStateEntry *StateEntry
expectedStateEntry *StateEntry
expectedReleaseInfo *ReleaseInfo
wantErr bool
}{
{
name: "return latest release given git extension is out of date and no state entry",
extCurrentVersion: "v0.1.0",
extLatestVersion: "v1.0.0",
extKind: extension.GitKind,
extURL: "http://example.com",
expectedStateEntry: &StateEntry{
CheckedForUpdateAt: now,
LatestRelease: ReleaseInfo{
Version: "v1.0.0",
URL: "http://example.com",
},
},
expectedReleaseInfo: &ReleaseInfo{
Version: "v1.0.0",
URL: "http://example.com",
},
},
{
name: "return latest release given git extension is out of date and state entry is old enough",
extCurrentVersion: "v0.1.0",
extLatestVersion: "v1.0.0",
extKind: extension.GitKind,
extURL: "http://example.com",
previousStateEntry: &StateEntry{
CheckedForUpdateAt: previousOldEnough,
LatestRelease: ReleaseInfo{
Version: "v0.1.0",
URL: "http://example.com",
},
},
expectedStateEntry: &StateEntry{
CheckedForUpdateAt: now,
LatestRelease: ReleaseInfo{
Version: "v1.0.0",
URL: "http://example.com",
},
},
expectedReleaseInfo: &ReleaseInfo{
Version: "v1.0.0",
URL: "http://example.com",
},
},
{
name: "return nothing given git extension is out of date but state entry is too recent",
extCurrentVersion: "v0.1.0",
extLatestVersion: "v1.0.0",
extKind: extension.GitKind,
extURL: "http://example.com",
previousStateEntry: &StateEntry{
CheckedForUpdateAt: previousTooSoon,
LatestRelease: ReleaseInfo{
Version: "v0.1.0",
URL: "http://example.com",
},
},
expectedStateEntry: &StateEntry{
CheckedForUpdateAt: previousTooSoon,
LatestRelease: ReleaseInfo{
Version: "v0.1.0",
URL: "http://example.com",
},
},
expectedReleaseInfo: nil,
},
{
name: "return latest release given binary extension is out of date and no state entry",
extCurrentVersion: "v0.1.0",
extLatestVersion: "v1.0.0",
extKind: extension.BinaryKind,
extURL: "http://example.com",
expectedStateEntry: &StateEntry{
CheckedForUpdateAt: now,
LatestRelease: ReleaseInfo{
Version: "v1.0.0",
URL: "http://example.com",
},
},
expectedReleaseInfo: &ReleaseInfo{
Version: "v1.0.0",
URL: "http://example.com",
},
},
{
name: "return latest release given binary extension is out of date and state entry is old enough",
extCurrentVersion: "v0.1.0",
extLatestVersion: "v1.0.0",
extKind: extension.BinaryKind,
extURL: "http://example.com",
previousStateEntry: &StateEntry{
CheckedForUpdateAt: previousOldEnough,
LatestRelease: ReleaseInfo{
Version: "v0.1.0",
URL: "http://example.com",
},
},
expectedStateEntry: &StateEntry{
CheckedForUpdateAt: now,
LatestRelease: ReleaseInfo{
Version: "v1.0.0",
URL: "http://example.com",
},
},
expectedReleaseInfo: &ReleaseInfo{
Version: "v1.0.0",
URL: "http://example.com",
},
},
{
name: "return nothing given binary extension is out of date but state entry is too recent",
extCurrentVersion: "v0.1.0",
extLatestVersion: "v1.0.0",
extKind: extension.BinaryKind,
extURL: "http://example.com",
previousStateEntry: &StateEntry{
CheckedForUpdateAt: previousTooSoon,
LatestRelease: ReleaseInfo{
Version: "v0.1.0",
URL: "http://example.com",
},
},
expectedStateEntry: &StateEntry{
CheckedForUpdateAt: previousTooSoon,
LatestRelease: ReleaseInfo{
Version: "v0.1.0",
URL: "http://example.com",
},
},
expectedReleaseInfo: nil,
},
{
name: "return nothing given local extension with no state entry",
extCurrentVersion: "v0.1.0",
extLatestVersion: "v1.0.0",
extKind: extension.LocalKind,
extURL: "http://example.com",
expectedStateEntry: nil,
expectedReleaseInfo: nil,
},
{
name: "return nothing given local extension despite state entry is old enough",
extCurrentVersion: "v0.1.0",
extLatestVersion: "v1.0.0",
extKind: extension.LocalKind,
extURL: "http://example.com",
previousStateEntry: &StateEntry{
CheckedForUpdateAt: previousOldEnough,
LatestRelease: ReleaseInfo{
Version: "v0.1.0",
URL: "http://example.com",
},
},
expectedStateEntry: &StateEntry{
CheckedForUpdateAt: previousOldEnough,
LatestRelease: ReleaseInfo{
Version: "v0.1.0",
URL: "http://example.com",
},
},
expectedReleaseInfo: nil,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
updateDir := t.TempDir()
em := &extensions.ExtensionManagerMock{
UpdateDirFunc: func(name string) string {
return filepath.Join(updateDir, name)
},
}

ext := &extensions.ExtensionMock{
NameFunc: func() string {
return "extension-update-test"
},
CurrentVersionFunc: func() string {
return tt.extCurrentVersion
},
LatestVersionFunc: func() string {
return tt.extLatestVersion
},
IsLocalFunc: func() bool {
return tt.extKind == extension.LocalKind
},
IsBinaryFunc: func() bool {
return tt.extKind == extension.BinaryKind
},
URLFunc: func() string {
return tt.extURL
},
}

// UpdateAvailable is arguably code under test but moq does not support partial mocks so this is a little brittle.
ext.UpdateAvailableFunc = func() bool {
if ext.IsLocal() {
panic("Local extensions do not get update notices")
}

// Actual extension versions should drive tests instead of managing UpdateAvailable separately.
current := ext.CurrentVersion()
latest := ext.LatestVersion()
return current != "" && latest != "" && current != latest
}

// Setup previous state file for test as necessary
stateFilePath := filepath.Join(em.UpdateDir(ext.Name()), "state.yml")
if tt.previousStateEntry != nil {
require.NoError(t, setStateEntry(stateFilePath, tt.previousStateEntry.CheckedForUpdateAt, tt.previousStateEntry.LatestRelease))
}

actual, err := CheckForExtensionUpdate(em, ext, now)
if tt.wantErr {
require.Error(t, err)
return
}

require.Equal(t, tt.expectedReleaseInfo, actual)

if tt.expectedStateEntry == nil {
require.NoFileExists(t, stateFilePath)
} else {
stateEntry, err := getStateEntry(stateFilePath)
require.NoError(t, err)
require.Equal(t, tt.expectedStateEntry, stateEntry)
}
})
}
}

func TestShouldCheckForExtensionUpdate(t *testing.T) {
tests := []struct {
name string
env map[string]string
expected bool
}{
{
name: "should not check when user has explicitly disable notifications",
env: map[string]string{
"GH_NO_UPDATE_NOTIFIER": "1",
},
expected: false,
},
{
name: "should not check when user is in codespace",
env: map[string]string{
"CODESPACES": "1",
},
expected: false,
},
{
name: "should not check when in GitHub Actions / Travis / Circle / Cirrus / GitLab / AppVeyor / CodeShip / dsari",
env: map[string]string{
"CI": "1",
},
expected: false,
},
{
name: "should not check when in Jenkins / TeamCity",
env: map[string]string{
"BUILD_NUMBER": "1",
},
expected: false,
},
{
name: "should not check when in TaskCluster / dsari",
env: map[string]string{
"RUN_ID": "1",
},
expected: false,
},
// TODO: Figure out how to refactor IsTerminal() to be testable
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
os.Clearenv()
for k, v := range tt.env {
os.Setenv(k, v)
}

actual := ShouldCheckForExtensionUpdate()
require.Equal(t, tt.expected, actual)
})
}
}

func tempFilePath() string {
file, err := os.CreateTemp("", "")
if err != nil {
Expand Down
Loading