Skip to content
This repository has been archived by the owner on Aug 16, 2022. It is now read-only.

test: add unit tests for Asset of mongodoc #129

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Conversation

yuuri111
Copy link
Contributor

Overview

reearth/reearth-visualizer#249

What I've done

add test for mongodoc

What I haven't done

How I tested

I did a following command
go test -v ./internal/infrastructure/mongo/mongodoc/.

Which point I want you to review particularly

Memo

internal/infrastructure/mongo/mongodoc/asset_test.go Outdated Show resolved Hide resolved
internal/infrastructure/mongo/mongodoc/asset_test.go Outdated Show resolved Hide resolved
internal/infrastructure/mongo/mongodoc/asset_test.go Outdated Show resolved Hide resolved
internal/infrastructure/mongo/mongodoc/asset_test.go Outdated Show resolved Hide resolved
internal/infrastructure/mongo/mongodoc/asset_test.go Outdated Show resolved Hide resolved
internal/infrastructure/mongo/mongodoc/asset_test.go Outdated Show resolved Hide resolved
@rot1024 rot1024 changed the title fix: add test for mongodoc test: add unit tests for Asset of mongodoc Mar 25, 2022
@codecov
Copy link

codecov bot commented Mar 25, 2022

Codecov Report

Merging #129 (e0abae6) into main (dccd9c7) will increase coverage by 0.09%.
The diff coverage is n/a.

❗ Current head e0abae6 differs from pull request most recent head 44eac9a. Consider uploading reports for the commit 44eac9a to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #129      +/-   ##
==========================================
+ Coverage   36.25%   36.34%   +0.09%     
==========================================
  Files         326      326              
  Lines       29069    29069              
==========================================
+ Hits        10538    10566      +28     
+ Misses      17576    17545      -31     
- Partials      955      958       +3     
Impacted Files Coverage Δ
internal/infrastructure/mongo/mongodoc/asset.go 65.11% <0.00%> (+65.11%) ⬆️

@yuuri111 yuuri111 requested a review from rot1024 March 25, 2022 08:30
Comment on lines 143 to 144
out := c.Consume(tt.args.raw)
assert.Equal(t, out == nil, tt.wantErr)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
out := c.Consume(tt.args.raw)
assert.Equal(t, out == nil, tt.wantErr)
err := c.Consume(tt.args.raw)
assert.Equal(t, err, tt.wantErr)
  • The variable name "out" is not appropriate because the return value of the consume method is an error.
  • Use a dedicated method to test for nil, such as assert.Nil, assert.NoError, etc. Here, however, we use Equal to test for both cases where an error occurs and where it does not. Try to add a test case that causes an error.
  • Refer to https://pkg.go.dev/github.com/stretchr/testify/assert to learn about assert functions

Comment on lines 116 to 127
type fields struct {
Rows []*asset.Asset
}
type args struct {
raw bson.Raw
}

tests := []struct {
name string
fields fields
args args
wantErr bool
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type fields struct {
Rows []*asset.Asset
}
type args struct {
raw bson.Raw
}
tests := []struct {
name string
fields fields
args args
wantErr bool
type args struct {
raw bson.Raw
}
tests := []struct {
name string
target *AssetConsumer
args args
want *AssetConsumer
wantErr bool
  • Code style: use target instead of fields
  • Since Consume is a method that mutates AssetConsumer itself, it must test itself expected after execution.
assert.Equal(t, tt.target, tt.want)

Comment on lines 64 to 91
newAsset := asset.New().
ID(aid).
CreatedAt(time.Time{}).
Team(tid).
Name("name").
Size(10).
URL("test").
ContentType("content type").
MustBuild()

tests := []struct {
name string
target *AssetDocument
want *asset.Asset
wantErr bool
}{
{
name: "asset model",
target: &AssetDocument{
ID: "01f2r7kg1fvvffp0gmexgy5hxy",
CreatedAt: time.Time{},
Team: "01f2r7kg1fvvffp0gmexgy5hxy",
Name: "name",
Size: 10,
URL: "test",
ContentType: "content type",
},
want: newAsset,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
newAsset := asset.New().
ID(aid).
CreatedAt(time.Time{}).
Team(tid).
Name("name").
Size(10).
URL("test").
ContentType("content type").
MustBuild()
tests := []struct {
name string
target *AssetDocument
want *asset.Asset
wantErr bool
}{
{
name: "asset model",
target: &AssetDocument{
ID: "01f2r7kg1fvvffp0gmexgy5hxy",
CreatedAt: time.Time{},
Team: "01f2r7kg1fvvffp0gmexgy5hxy",
Name: "name",
Size: 10,
URL: "test",
ContentType: "content type",
},
want: newAsset,
now := time.Now()
assetID := id.NewAssetID()
teamID := id.NewTeamID()
tests := []struct {
name string
target *AssetDocument
want *asset.Asset
wantErr bool
}{
{
name: "asset model",
target: &AssetDocument{
ID: assetID.String(),
CreatedAt: now,
Team: teamID.String(),
Name: "name",
Size: 10,
URL: "test",
ContentType: "content type",
},
want: asset.New().
ID(assetID).
CreatedAt(now).
Team(teamID).
Name("name").
Size(10).
URL("test").
ContentType("content type").
MustBuild(),

It is easier to duplicate test cases and add various test cases if Asset is initialized for each test case.

Comment on lines 18 to 44
newAsset := asset.New().
NewID().
Team(id.NewTeamID()).
Name("test").
Size(10).
URL("test_url").
MustBuild()

tests := []struct {
name string
args args
want *AssetDocument
want1 string
}{
{
name: "new asset",
args: args{
asset: newAsset,
},
want: &AssetDocument{
ID: newAsset.ID().String(),
CreatedAt: newAsset.CreatedAt(),
Team: newAsset.Team().String(),
Name: newAsset.Name(),
Size: newAsset.Size(),
URL: newAsset.URL(),
ContentType: newAsset.ContentType(),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
newAsset := asset.New().
NewID().
Team(id.NewTeamID()).
Name("test").
Size(10).
URL("test_url").
MustBuild()
tests := []struct {
name string
args args
want *AssetDocument
want1 string
}{
{
name: "new asset",
args: args{
asset: newAsset,
},
want: &AssetDocument{
ID: newAsset.ID().String(),
CreatedAt: newAsset.CreatedAt(),
Team: newAsset.Team().String(),
Name: newAsset.Name(),
Size: newAsset.Size(),
URL: newAsset.URL(),
ContentType: newAsset.ContentType(),
assetID := id.NewAssetID()
teamID := id.NewTeamID()
now := time.Now()
tests := []struct {
name string
args args
want *AssetDocument
want1 string
}{
{
name: "new asset",
args: args{
asset: asset.New().
ID(assetID).
Team(teamID).
CreatedAt(now).
Name("test").
Size(10).
URL("test_url").
ContentType("application/json").
MustBuild(),
},
want: &AssetDocument{
ID: assetID.String(),
CreatedAt: now,
Team: teamID.String(),
Name: "test",
Size: 10,
URL: "test_url",
ContentType: "application/json",

It is easier to increase the number of test cases by generating IDs in advance and using them rather than using the Asset itself.

}

func TestAssetDocument_Model(t *testing.T) {
aid, _ := id.AssetIDFrom("01f2r7kg1fvvffp0gmexgy5hxy")
Copy link
Member

@rot1024 rot1024 Mar 28, 2022

Choose a reason for hiding this comment

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

id.Must*ID is also available

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants