Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 3 additions & 3 deletions packages/trace.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ func (p tracingInstalledPackagesProvider) GetInstalledPackages(ctx context.Conte
cancel()
result := <-resultChannel

osinfo, err := p.osInfoProvider.GetOSInfo(ctx)
if err != nil {
clog.Errorf(ctx, "GetOSInfo() error: %v", err)
osinfo, oserr := p.osInfoProvider.GetOSInfo(ctx)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for this change ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see, an error is silenced in the scenario where GetInstalledPackages returns error

if oserr != nil {
clog.Errorf(ctx, "GetOSInfo() error: %v", oserr)
}
logTraceResult(ctx, result, duration, osinfo)

Expand Down
168 changes: 168 additions & 0 deletions packages/trace_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
// Copyright 2026 Google Inc. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package packages

import (
"bytes"
"context"
"errors"
"io"
"reflect"
"testing"
"time"

"github.com/GoogleCloudPlatform/guest-logging-go/logger"
"github.com/GoogleCloudPlatform/osconfig/osinfo"
"github.com/GoogleCloudPlatform/osconfig/util/utiltest"
"github.com/golang/mock/gomock"
)

// mockInstalledPackagesProvider is a gomock implementation of InstalledPackagesProvider.
type mockInstalledPackagesProvider struct {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will gomock work here ? Please avoid creating stubs manually if they can be generated with a gomock

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will check. Converted this PR to drfat for now, it requires an additional work

ctrl *gomock.Controller
recorder *mockInstalledPackagesProviderMockRecorder
}

type mockInstalledPackagesProviderMockRecorder struct {
mock *mockInstalledPackagesProvider
}

func newMockInstalledPackagesProvider(ctrl *gomock.Controller) *mockInstalledPackagesProvider {
mock := &mockInstalledPackagesProvider{ctrl: ctrl}
mock.recorder = &mockInstalledPackagesProviderMockRecorder{mock}
return mock
}

func (m *mockInstalledPackagesProvider) EXPECT() *mockInstalledPackagesProviderMockRecorder {
return m.recorder
}

func (m *mockInstalledPackagesProvider) GetInstalledPackages(ctx context.Context) (Packages, error) {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "GetInstalledPackages", ctx)
ret0, _ := ret[0].(Packages)
ret1, _ := ret[1].(error)
return ret0, ret1
}

func (mr *mockInstalledPackagesProviderMockRecorder) GetInstalledPackages(ctx interface{}) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetInstalledPackages", reflect.TypeOf((*mockInstalledPackagesProvider)(nil).GetInstalledPackages), ctx)
}

// mockOSInfoProvider is a gomock implementation of osinfo.Provider.
type mockOSInfoProvider struct {
ctrl *gomock.Controller
recorder *mockOSInfoProviderMockRecorder
}

type mockOSInfoProviderMockRecorder struct {
mock *mockOSInfoProvider
}

func newMockOSInfoProvider(ctrl *gomock.Controller) *mockOSInfoProvider {
mock := &mockOSInfoProvider{ctrl: ctrl}
mock.recorder = &mockOSInfoProviderMockRecorder{mock}
return mock
}

func (m *mockOSInfoProvider) EXPECT() *mockOSInfoProviderMockRecorder {
return m.recorder
}

func (m *mockOSInfoProvider) GetOSInfo(ctx context.Context) (osinfo.OSInfo, error) {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "GetOSInfo", ctx)
ret0, _ := ret[0].(osinfo.OSInfo)
ret1, _ := ret[1].(error)
return ret0, ret1
}

func (mr *mockOSInfoProviderMockRecorder) GetOSInfo(ctx interface{}) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetOSInfo", reflect.TypeOf((*mockOSInfoProvider)(nil).GetOSInfo), ctx)
}

// TestTracingInstalledPackagesProvider verifies that the tracing decorator
// correctly handles results and errors from the underlying providers.
func TestTracingInstalledPackagesProvider(t *testing.T) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure test is actually testing any behavior of the TracingInstalledPackagesProvider, TracingInstalledPackagesProvider created for the purpose to measure resources overhead of the specific provider, what i see test is testing is just pass through behavior.

testPkgs := Packages{Yum: []*PkgInfo{{Name: "pkg1"}}}
testInfo := osinfo.OSInfo{Hostname: "test-host"}
// logPattern matches the trace log output format, for example:
// GetInstalledPackages: 0.111s, memory +0.00 MB (=10.26-10.26), peak 10.26 MB, mean 10.26 MB (2 samples), OS: @, hostname: test-host
logPattern := `GetInstalledPackages: \d+\.\d+s, memory [+-]?\d+\.\d+ MB \(=\d+\.\d+-\d+\.\d+\), peak \d+\.\d+ MB, mean \d+\.\d+ MB \(\d+ samples\), OS: .*@.*, hostname: test-host`

tests := []struct {
name string
tracedErr error
osInfoErr error
wantPkgs Packages
wantErr error
}{
{
name: "get packages without errors",
tracedErr: nil,
osInfoErr: nil,
wantPkgs: testPkgs,
wantErr: nil,
},
{
name: "get packages with errors",
tracedErr: errors.New("traced provider error"),
osInfoErr: nil,
wantPkgs: Packages{},
wantErr: errors.New("traced provider error"),
},
{
name: "osinfo provider returns error",
tracedErr: nil,
osInfoErr: errors.New("osinfo error"),
wantPkgs: testPkgs,
wantErr: nil,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
var buf bytes.Buffer
_ = logger.Init(context.Background(), logger.LogOpts{LoggerName: "trace_test", Debug: true, Writers: []io.Writer{&buf}})

mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()

tp := newMockInstalledPackagesProvider(mockCtrl)
op := newMockOSInfoProvider(mockCtrl)

// Verify sequence and allow time for TraceMemory to sample.
call1 := tp.EXPECT().GetInstalledPackages(gomock.Any()).DoAndReturn(func(ctx context.Context) (Packages, error) {
// Wait at least 110ms to ensure TraceMemory (100ms interval) samples at least once.
time.Sleep(110 * time.Millisecond)
return tt.wantPkgs, tt.tracedErr
}).Times(1)

op.EXPECT().GetOSInfo(gomock.Any()).After(call1).Return(testInfo, tt.osInfoErr).Times(1)

provider := TracingInstalledPackagesProvider(tp, op)
gotPkgs, err := provider.GetInstalledPackages(context.Background())

if !reflect.DeepEqual(gotPkgs, tt.wantPkgs) {
t.Errorf("GetInstalledPackages() gotPkgs = %v, want %v", gotPkgs, tt.wantPkgs)
}

utiltest.AssertErrorMatch(t, err, tt.wantErr)
utiltest.AssertFormatMatch(t, buf.String(), logPattern)
})
}
}
13 changes: 13 additions & 0 deletions util/utiltest/utiltest.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,25 @@ import (
"os"
"path/filepath"
"reflect"
"regexp"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/kr/pretty"
)

// AssertFormatMatch verifies that the gotString matches the wantFormat regular expression.
func AssertFormatMatch(t *testing.T, gotString string, wantFormat string) {
t.Helper()
matched, err := regexp.MatchString(wantFormat, gotString)
if err != nil {
t.Fatalf("regexp.MatchString(%q, %q) err: %v", wantFormat, gotString, err)
}
if !matched {
t.Errorf("string %q does not match format %q", gotString, wantFormat)
}
}

// BytesFromFile returns file as bytes; propagates err (e.g. file does not exist) as test failure reason
func BytesFromFile(t *testing.T, filepath string) []byte {
bytes, err := os.ReadFile(filepath)
Expand Down
Loading