Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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.

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

MIGRATION ISSUE: replacement for awserr.New() when creating errors for unit tests #2857

Closed
2 tasks done
erikpaasonen opened this issue Oct 29, 2024 · 3 comments
Closed
2 tasks done
Assignees

Comments

@erikpaasonen
Copy link

Pre-Migration Checklist

Go Version Used

Go 1.23

Describe the Migration Issue

Seeking the upgrade path for awserr.New() with respect to creating errors which can be unwrapped by errors.As() as recommended by the migration guide.

A simple, clear scenario is a function which creates a CloudWatch log group as needed. During certain well-understood race conditions the log group very well may exist by the time the code calls .CreateLogGroup(). I need to swallow the specific error condition when the log group already exists, and pass-through the error on all other error types. In order to unit test this function, I need to generate an error which emulates as perfectly as possible the actual error received at runtime.

In Go SDK v1 this was easy: awserr.New(). The migration guide's section on error handling as well as the Go SDK v2 error handling guide say that the Smithy-Go error wrapping is intended to completely replace the awserr package. I can find guidance on how to check these errors but unfortunately haven't found any yet on how to create or emulate them.

Code Comparison

Function snippet following the migration guide for error unwrapping and taking distinct action for a particular error code:

func (f FooStruct) MyCreateLogGrp(logGroupName string) (err error) {
  _, err = f.client.CreateLogGroup(context.TODO(), &cloudwatchlogs.CreateLogGroupInput{
    LogGroupName: aws.String(logGroupName),
  })
  if err != nil {
    var s smithy.APIError
    if errors.As(err, &s) {
      var raee *types.ResourceAlreadyExistsException
      if s.ErrorCode() == raee.ErrorCode() {
        log.Info("swallowing error")
      } else {
        log.Error("something happened")
      }
    } else {
      log.Error("did not satisfy smithy.APIError")
    }
  }
  return
}

also tried the simpler case to the CWL type directly, with effectively the same results:

func (f FooStruct) MyCreateLogGrp(logGroupName string) (err error) {
  _, err = f.client.CreateLogGroup(context.TODO(), &cloudwatchlogs.CreateLogGroupInput{
    LogGroupName: aws.String(logGroupName),
  })
  if err != nil {
    var raee *types.ResourceAlreadyExistsException
    if errors.As(err, raee) {
      log.Info("swallowing error")
    } else {
      log.Error("did not satisfy types.ResourceAlreadyExistsException")
    }
  }
  return
}

V1 test code which has worked fine:

func TestMyCreateLogGrp(t *testing.T) {  
  mockController := gomock.NewController(t)
  defer mockController.Finish()
  mockCloudWatchLogs := mockcwl.NewMockCloudWatchLogsAPI(mockController)
  mockCloudWatchLogs.EXPECT().CreateLogGroup(context.TODO(), gomock.Any()).Return(
    &cloudwatchlogs.CreateLogGroupOutput{},
    awserr.New(cloudwatchlogs.ErrCodeResourceAlreadyExistsException, "Log group already exists", nil),
  )
  ...
}

V2 test code returning a custom struct which ought to satisfy Smithy-Go, but unfortunately not working:

type mockApiError struct {
    ErrorCode string
    ErrorMsg  string
    Fault     smithy.ErrorFault
}

func TestMyCreateLogGrp(t *testing.T) {  
  mockController := gomock.NewController(t)
  defer mockController.Finish()
  mockCloudWatchLogs := mockcwlv2.NewMockCloudWatchLogsAPI(mockController)
  var raee types.ResourceAlreadyExistsException
  mockCloudWatchLogs.EXPECT().CreateLogGroup(context.TODO(), gomock.Any()).Return(
    &cloudwatchlogs.CreateLogGroupOutput{},
    mockApiError{
      ErrorCode: raee.ErrorCode(),
      ErrorMsg:  raee.ErrorMessage(),
      Fault:     raee.ErrorFault(),
    },
  )
  ...
}

also tried logging an SDK v2 error in an isolated environment, and copying-pasting its error string to errors.New():

func TestMyCreateLogGrp(t *testing.T) {  
  mockController := gomock.NewController(t)
  defer mockController.Finish()
  mockCloudWatchLogs := mockcwlv2.NewMockCloudWatchLogsAPI(mockController)
  mockCloudWatchLogs.EXPECT().CreateLogGroup(context.TODO(), gomock.Any()).Return(
    &cloudwatchlogs.CreateLogGroupOutput{},
    errors.New("operation error CloudWatch Logs: CreateLogGroup, https response error StatusCode: 400, RequestID: b978xxxx-xxxx-xxxx-xxxx-xxxx1ef4cf89, ResourceAlreadyExistsException: The specified log group already exists"),
  )
  ...
}

Observed Differences/Errors

In both of the above V2 variations, the code yields the log.Error("did not satisfy...") instead of the log.Info("swallowing error") as expected. Ultimately I'm no longer able to fully unit test this function with V2 now that awserr.New() has gone away.

Additional Context

No response

@erikpaasonen erikpaasonen added needs-triage This issue or PR still needs to be triaged. v1-v2-inconsistency v1-v2-inconsistency Behavior has changed from v1 to v2, or feature is missing altogether labels Oct 29, 2024
@lucix-aws
Copy link
Contributor

V2 test code returning a custom struct which ought to satisfy Smithy-Go, but unfortunately not working:

type mockApiError struct {
    ErrorCode string
    ErrorMsg  string
    Fault     smithy.ErrorFault
}

This doesn't actually satisfy smithy.APIError though - you don't have the corresponding methods on that struct, unless your sample just doesn't show them (don't see how it could, though - you have an ErrorCode field, so you can't have an ErrorCode() method too)

Double-check that you're satisfying the interface w/

var _ smithy.APIError = (*mockApiError)(nil)

You want something like

type mockError struct {
  code, msg string
  fault smithy.ErrorFault
}

func(m *mockError) ErrorCode() string { return m.code }
func(m *mockError) ErrorMessage() string { return m.msg }
func(m *mockError) ErrorFault() smithy.ErrorFault { return m.fault }
func(m *mockError) Error() string { return "..." }

var _ smithy.APIError = (*mockError)(nil)

You can also just use the concrete smithy.GenericAPIError, which satisfies smithy.APIError.

@erikpaasonen
Copy link
Author

Thank you for the reply. I had not made any effort to satisfy the interface, only matched the struct fields. So the path forward seems to be to satisfy the interface.

When I use smithy.GenericAPIError as in:

var raee types.ResourceAlreadyExistsException
mockCloudWatchLogs.EXPECT().CreateLogGroup(context.TODO(), gomock.Any()).Return(
    &cloudwatchlogs.CreateLogGroupOutput{},
    smithy.GenericAPIError{
        Code:    raee.ErrorCode(),
        Message: raee.ErrorMessage(),
        Fault:   raee.ErrorFault(),
    },
)

I get this error:

wrong type of argument 1 to Return for *mocks.MockCloudWatchLogsAPI.CreateLogGroup: smithy.GenericAPIError is not assignable to error

Am I using it incorrectly?

@lucix-aws
Copy link
Contributor

lucix-aws commented Oct 29, 2024

So the path forward seems to be to satisfy the interface.

Correct, this is the case across the board in Go. "Is this an X" checks at runtime are idiomatically done via checking if something satisfies an interface. (that's what errors.As is doing under the hood)

I think it needs to be &smithy.GenericAPIError{} (& for pointer-to, you're just passing a value) since the methods are declared with pointer receivers. I'm surprised the compiler didn't state that explicitly.

@RanVaknin RanVaknin self-assigned this Oct 29, 2024
@RanVaknin RanVaknin removed needs-triage This issue or PR still needs to be triaged. v1-v2-inconsistency v1-v2-inconsistency Behavior has changed from v1 to v2, or feature is missing altogether labels Oct 29, 2024
@aws aws locked and limited conversation to collaborators Oct 29, 2024
@RanVaknin RanVaknin converted this issue into discussion #2858 Oct 29, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants