You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2021/01/17 12:50:57 UTC
[GitHub] [apisix-dashboard] starsz opened a new pull request #1318: feat: add store mock
starsz opened a new pull request #1318:
URL: https://github.com/apache/apisix-dashboard/pull/1318
Please answer these questions before submitting a pull request
- Why submit this pull request?
- [ ] Bugfix
- [x] New feature provided
- [ ] Improve performance
- [ ] Backport patches
- Related issues
#1161
___
### New feature or improvement
- Describe the details and related test reports.
Add store mock so that we can use this instead of the etcd dependency in unit test.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] codecov-io commented on pull request #1318: feat: add store mock
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #1318:
URL: https://github.com/apache/apisix-dashboard/pull/1318#issuecomment-761807941
# [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1318?src=pr&el=h1) Report
> Merging [#1318](https://codecov.io/gh/apache/apisix-dashboard/pull/1318?src=pr&el=desc) (509be5b) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/754f5598706fe5dfe9a09bf0d51d0701406c692c?el=desc) (754f559) will **increase** coverage by `1.85%`.
> The diff coverage is `64.42%`.
[](https://codecov.io/gh/apache/apisix-dashboard/pull/1318?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #1318 +/- ##
==========================================
+ Coverage 44.19% 46.05% +1.85%
==========================================
Files 31 32 +1
Lines 1948 2078 +130
==========================================
+ Hits 861 957 +96
- Misses 975 1002 +27
- Partials 112 119 +7
```
| [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/1318?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [api/internal/core/store/mock\_interface.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1318/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmUvbW9ja19pbnRlcmZhY2UuZ28=) | `32.20% <32.20%> (ø)` | |
| [api/internal/core/store/store\_mock.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1318/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmUvc3RvcmVfbW9jay5nbw==) | `85.55% <85.55%> (+85.55%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1318?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1318?src=pr&el=footer). Last update [754f559...509be5b](https://codecov.io/gh/apache/apisix-dashboard/pull/1318?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] tokers commented on a change in pull request #1318: feat: add store mock
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #1318:
URL: https://github.com/apache/apisix-dashboard/pull/1318#discussion_r559323045
##########
File path: api/internal/core/store/mock_interface.go
##########
@@ -0,0 +1,131 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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 store
+
+import (
+ "context"
+
+ "github.com/stretchr/testify/mock"
+)
+
+type MockInterface struct {
+ mock.Mock
+}
+
+func (m *MockInterface) Get(key string) (interface{}, error) {
+ ret := m.Called(key)
+
+ if rf, ok := ret.Get(0).(func(key string) (interface{}, error)); ok {
Review comment:
Better to add some comments to mark that the file is generated automically, just like kubebuilder. Otherwise we cannot distinguish the code source.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] starsz commented on a change in pull request #1318: feat: add store mock
Posted by GitBox <gi...@apache.org>.
starsz commented on a change in pull request #1318:
URL: https://github.com/apache/apisix-dashboard/pull/1318#discussion_r559278753
##########
File path: api/internal/core/store/store_mock_test.go
##########
@@ -0,0 +1,200 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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 store
+
+import (
+ "context"
+ "fmt"
+ "testing"
+
+ "github.com/apisix/manager-api/internal/core/entity"
+ "github.com/apisix/manager-api/internal/utils"
+ "github.com/shiningrush/droplet/data"
+ "github.com/stretchr/testify/assert"
+)
+
+type testObj struct {
+ entity.BaseInfo
+ Name string
+ Value string
+}
+
+func keyFunc(obj interface{}) string {
+ return utils.InterfaceToString(obj.(*testObj).Name)
+}
+
+func TestMock_Create(t *testing.T) {
+ ctx := context.Background()
+ m := NewMock(keyFunc, nil)
+
+ // create success
+ ret, err := m.Create(ctx, &testObj{Name: "test"})
+ assert.Nil(t, err)
+ assert.Equal(t, "test", ret.(*testObj).Name)
+
+ // create failed, "key is required"
+ ret, err = m.Create(ctx, &testObj{Name: ""})
+ assert.Equal(t, fmt.Errorf("key is required"), err)
Review comment:
Updated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] juzhiyuan commented on pull request #1318: feat: add store mock
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on pull request #1318:
URL: https://github.com/apache/apisix-dashboard/pull/1318#issuecomment-762560004
ping @starsz Please resolve those conflicts.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] ShiningRush commented on pull request #1318: feat: add store mock
Posted by GitBox <gi...@apache.org>.
ShiningRush commented on pull request #1318:
URL: https://github.com/apache/apisix-dashboard/pull/1318#issuecomment-761929542
Why we need this PR?I think the original mock store is enough, please have a look at #859
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] starsz closed pull request #1318: feat: add store mock
Posted by GitBox <gi...@apache.org>.
starsz closed pull request #1318:
URL: https://github.com/apache/apisix-dashboard/pull/1318
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] ShiningRush commented on pull request #1318: feat: add store mock
Posted by GitBox <gi...@apache.org>.
ShiningRush commented on pull request #1318:
URL: https://github.com/apache/apisix-dashboard/pull/1318#issuecomment-761993164
> Original mock doesn't have the store ability, so we need to provide mock data in every unit test.
Yes, this is the job that stub module should do, if you want to reuse stub data, you should just extract those data instead of the logic.
Put the store logic into mock object will cause `Shotgun Surgery` that when we change the logic of store, we need also change the mock object's loigc.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] juzhiyuan commented on pull request #1318: feat: add store mock
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on pull request #1318:
URL: https://github.com/apache/apisix-dashboard/pull/1318#issuecomment-766528141
is this urgent?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] starsz commented on a change in pull request #1318: feat: add store mock
Posted by GitBox <gi...@apache.org>.
starsz commented on a change in pull request #1318:
URL: https://github.com/apache/apisix-dashboard/pull/1318#discussion_r559278832
##########
File path: api/internal/core/store/store_mock.go
##########
@@ -18,48 +18,184 @@ package store
import (
"context"
+ "encoding/json"
+ "fmt"
+ "sort"
+ "github.com/apisix/manager-api/internal/core/entity"
+ "github.com/shiningrush/droplet/data"
"github.com/stretchr/testify/mock"
)
-type MockInterface struct {
- mock.Mock
+type Mock struct {
+ MockInterface
+ storage map[string]interface{}
+ validator Validator
+ keyFunc func(obj interface{}) string
}
-func (m *MockInterface) Get(key string) (interface{}, error) {
- ret := m.Mock.Called(key)
- return ret.Get(0), ret.Error(1)
+func NewMock(keyFunc func(obj interface{}) string, validator Validator) *Mock {
+ m := &Mock{
+ storage: make(map[string]interface{}),
+ validator: validator,
+ keyFunc: keyFunc,
+ }
+
+ m.onCreate()
+ m.onGet()
+ m.onUpdate()
+ m.onList()
+ m.onBatchDelete()
+ return m
+}
+
+func (m *Mock) create(ctx context.Context, obj interface{}) (interface{}, error) {
+ if setter, ok := obj.(entity.BaseInfoSetter); ok {
+ info := setter.GetBaseInfo()
+ info.Creating()
+ }
+
+ if m.validator != nil {
+ if err := m.validator.Validate(obj); err != nil {
+ return nil, err
+ }
+ }
+
+ key := m.keyFunc(obj)
+ if key == "" {
+ return nil, fmt.Errorf("key is required")
+ }
+ if _, exist := m.storage[key]; exist {
+ return nil, fmt.Errorf("key: %s is conflicted", key)
+ }
+
+ _, err := json.Marshal(obj)
+ if err != nil {
+ return nil, fmt.Errorf("json marshal failed: %s", err)
+ }
+
+ m.storage[key] = obj
+ return obj, nil
}
-func (m *MockInterface) List(input ListInput) (*ListOutput, error) {
- ret := m.Called(input)
+func (m *Mock) onCreate() {
+ m.On("Create", mock.Anything, mock.Anything).Return(m.create, nil)
+}
- var (
- r0 *ListOutput
- r1 error
- )
+func (m *Mock) onGet() {
+ rf := func(key string) (interface{}, error) {
+ r, exist := m.storage[key]
+ if !exist {
+ return nil, data.ErrNotFound
+ }
- if rf, ok := ret.Get(0).(func(ListInput) *ListOutput); ok {
- r0 = rf(input)
- } else {
- r0 = ret.Get(0).(*ListOutput)
+ return r, nil
}
- r1 = ret.Error(1)
- return r0, r1
+ m.On("Get", mock.Anything).Return(rf, nil)
}
-func (m *MockInterface) Create(ctx context.Context, obj interface{}) (interface{}, error) {
- ret := m.Mock.Called(ctx, obj)
- return ret.Get(0), ret.Error(1)
+func (m *Mock) onList() {
+ rf := func(input ListInput) (*ListOutput, error) {
+ var ret = make([]interface{}, 0)
+ for _, v := range m.storage {
+ value := v
+ if input.Predicate != nil && !input.Predicate(v) {
+ continue
+ }
+
+ if input.Format != nil {
+ value = input.Format(v)
+ }
+
+ ret = append(ret, value)
+ }
+
+ output := &ListOutput{
+ Rows: ret,
+ TotalSize: len(ret),
+ }
+
+ if input.Less == nil {
+ input.Less = defLessFunc
+ }
+
+ sort.Slice(output.Rows, func(i, j int) bool {
+ return input.Less(output.Rows[i], output.Rows[j])
+ })
+
+ if input.PageSize > 0 && input.PageNumber > 0 {
+ skipCount := (input.PageNumber - 1) * input.PageSize
+ if skipCount > output.TotalSize {
+ output.Rows = []interface{}{}
+ return output, nil
+ }
+
+ endIdx := skipCount + input.PageSize
+ if endIdx >= output.TotalSize {
+ output.Rows = ret[skipCount:]
+ return output, nil
+ }
+ output.Rows = ret[skipCount:endIdx]
+ }
+
+ return output, nil
+ }
+
+ m.On("List", mock.Anything).Return(rf, nil)
}
-func (m *MockInterface) Update(ctx context.Context, obj interface{}, createOnFail bool) error {
- ret := m.Mock.Called(ctx, obj, createOnFail)
- return ret.Error(0)
+func (m *Mock) onUpdate() {
+ rf := func(ctx context.Context, obj interface{}, createIfNotExist bool) error {
+ if m.validator != nil {
+ if err := m.validator.Validate(obj); err != nil {
+ return err
+ }
+ }
+
+ key := m.keyFunc(obj)
+ if key == "" {
+ return fmt.Errorf("key is required")
Review comment:
Updated.Thank you very much.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] starsz commented on pull request #1318: feat: add store mock
Posted by GitBox <gi...@apache.org>.
starsz commented on pull request #1318:
URL: https://github.com/apache/apisix-dashboard/pull/1318#issuecomment-762561224
@ShiningRush @nic-chen Thanks for your advice.
Sorry, I don't think too much about it.
Maybe we can discuss it.
@juzhiyuan This PR is WIP now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] tokers commented on a change in pull request #1318: feat: add store mock
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #1318:
URL: https://github.com/apache/apisix-dashboard/pull/1318#discussion_r559270738
##########
File path: api/internal/core/store/mock_interface.go
##########
@@ -0,0 +1,131 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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 store
+
+import (
+ "context"
+
+ "github.com/stretchr/testify/mock"
+)
+
+type MockInterface struct {
+ mock.Mock
+}
+
+func (m *MockInterface) Get(key string) (interface{}, error) {
+ ret := m.Called(key)
+
+ if rf, ok := ret.Get(0).(func(key string) (interface{}, error)); ok {
Review comment:
Could we just use a type assert switch statement here? The type assertions are all around.
##########
File path: api/internal/core/store/store_mock.go
##########
@@ -18,48 +18,184 @@ package store
import (
"context"
+ "encoding/json"
+ "fmt"
+ "sort"
+ "github.com/apisix/manager-api/internal/core/entity"
+ "github.com/shiningrush/droplet/data"
"github.com/stretchr/testify/mock"
)
-type MockInterface struct {
- mock.Mock
+type Mock struct {
+ MockInterface
+ storage map[string]interface{}
+ validator Validator
+ keyFunc func(obj interface{}) string
}
-func (m *MockInterface) Get(key string) (interface{}, error) {
- ret := m.Mock.Called(key)
- return ret.Get(0), ret.Error(1)
+func NewMock(keyFunc func(obj interface{}) string, validator Validator) *Mock {
+ m := &Mock{
+ storage: make(map[string]interface{}),
+ validator: validator,
+ keyFunc: keyFunc,
+ }
+
+ m.onCreate()
+ m.onGet()
+ m.onUpdate()
+ m.onList()
+ m.onBatchDelete()
+ return m
+}
+
+func (m *Mock) create(ctx context.Context, obj interface{}) (interface{}, error) {
+ if setter, ok := obj.(entity.BaseInfoSetter); ok {
+ info := setter.GetBaseInfo()
+ info.Creating()
+ }
+
+ if m.validator != nil {
+ if err := m.validator.Validate(obj); err != nil {
+ return nil, err
+ }
+ }
+
+ key := m.keyFunc(obj)
+ if key == "" {
+ return nil, fmt.Errorf("key is required")
+ }
+ if _, exist := m.storage[key]; exist {
+ return nil, fmt.Errorf("key: %s is conflicted", key)
+ }
+
+ _, err := json.Marshal(obj)
+ if err != nil {
+ return nil, fmt.Errorf("json marshal failed: %s", err)
+ }
+
+ m.storage[key] = obj
+ return obj, nil
}
-func (m *MockInterface) List(input ListInput) (*ListOutput, error) {
- ret := m.Called(input)
+func (m *Mock) onCreate() {
+ m.On("Create", mock.Anything, mock.Anything).Return(m.create, nil)
+}
- var (
- r0 *ListOutput
- r1 error
- )
+func (m *Mock) onGet() {
+ rf := func(key string) (interface{}, error) {
+ r, exist := m.storage[key]
+ if !exist {
+ return nil, data.ErrNotFound
+ }
- if rf, ok := ret.Get(0).(func(ListInput) *ListOutput); ok {
- r0 = rf(input)
- } else {
- r0 = ret.Get(0).(*ListOutput)
+ return r, nil
}
- r1 = ret.Error(1)
- return r0, r1
+ m.On("Get", mock.Anything).Return(rf, nil)
}
-func (m *MockInterface) Create(ctx context.Context, obj interface{}) (interface{}, error) {
- ret := m.Mock.Called(ctx, obj)
- return ret.Get(0), ret.Error(1)
+func (m *Mock) onList() {
+ rf := func(input ListInput) (*ListOutput, error) {
+ var ret = make([]interface{}, 0)
+ for _, v := range m.storage {
+ value := v
+ if input.Predicate != nil && !input.Predicate(v) {
+ continue
+ }
+
+ if input.Format != nil {
+ value = input.Format(v)
+ }
+
+ ret = append(ret, value)
+ }
+
+ output := &ListOutput{
+ Rows: ret,
+ TotalSize: len(ret),
+ }
+
+ if input.Less == nil {
+ input.Less = defLessFunc
+ }
+
+ sort.Slice(output.Rows, func(i, j int) bool {
+ return input.Less(output.Rows[i], output.Rows[j])
+ })
+
+ if input.PageSize > 0 && input.PageNumber > 0 {
+ skipCount := (input.PageNumber - 1) * input.PageSize
+ if skipCount > output.TotalSize {
+ output.Rows = []interface{}{}
+ return output, nil
+ }
+
+ endIdx := skipCount + input.PageSize
+ if endIdx >= output.TotalSize {
+ output.Rows = ret[skipCount:]
+ return output, nil
+ }
+ output.Rows = ret[skipCount:endIdx]
+ }
+
+ return output, nil
+ }
+
+ m.On("List", mock.Anything).Return(rf, nil)
}
-func (m *MockInterface) Update(ctx context.Context, obj interface{}, createOnFail bool) error {
- ret := m.Mock.Called(ctx, obj, createOnFail)
- return ret.Error(0)
+func (m *Mock) onUpdate() {
+ rf := func(ctx context.Context, obj interface{}, createIfNotExist bool) error {
+ if m.validator != nil {
+ if err := m.validator.Validate(obj); err != nil {
+ return err
+ }
+ }
+
+ key := m.keyFunc(obj)
+ if key == "" {
+ return fmt.Errorf("key is required")
Review comment:
Ditto, just use `errors.New` is enough.
##########
File path: api/internal/core/store/store_mock.go
##########
@@ -18,48 +18,184 @@ package store
import (
"context"
+ "encoding/json"
+ "fmt"
+ "sort"
+ "github.com/apisix/manager-api/internal/core/entity"
+ "github.com/shiningrush/droplet/data"
"github.com/stretchr/testify/mock"
)
-type MockInterface struct {
- mock.Mock
+type Mock struct {
+ MockInterface
Review comment:
It would be better to keep an empty line after the embed object.
##########
File path: api/internal/core/store/store_mock_test.go
##########
@@ -0,0 +1,200 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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 store
+
+import (
+ "context"
+ "fmt"
+ "testing"
+
+ "github.com/apisix/manager-api/internal/core/entity"
+ "github.com/apisix/manager-api/internal/utils"
+ "github.com/shiningrush/droplet/data"
+ "github.com/stretchr/testify/assert"
+)
+
+type testObj struct {
+ entity.BaseInfo
Review comment:
Keep an empty line after the embed object.
##########
File path: api/internal/core/store/store_mock.go
##########
@@ -18,48 +18,184 @@ package store
import (
"context"
+ "encoding/json"
+ "fmt"
+ "sort"
+ "github.com/apisix/manager-api/internal/core/entity"
+ "github.com/shiningrush/droplet/data"
"github.com/stretchr/testify/mock"
)
-type MockInterface struct {
- mock.Mock
+type Mock struct {
+ MockInterface
+ storage map[string]interface{}
+ validator Validator
+ keyFunc func(obj interface{}) string
}
-func (m *MockInterface) Get(key string) (interface{}, error) {
- ret := m.Mock.Called(key)
- return ret.Get(0), ret.Error(1)
+func NewMock(keyFunc func(obj interface{}) string, validator Validator) *Mock {
+ m := &Mock{
+ storage: make(map[string]interface{}),
+ validator: validator,
+ keyFunc: keyFunc,
+ }
+
+ m.onCreate()
+ m.onGet()
+ m.onUpdate()
+ m.onList()
+ m.onBatchDelete()
+ return m
+}
+
+func (m *Mock) create(ctx context.Context, obj interface{}) (interface{}, error) {
+ if setter, ok := obj.(entity.BaseInfoSetter); ok {
+ info := setter.GetBaseInfo()
+ info.Creating()
+ }
+
+ if m.validator != nil {
+ if err := m.validator.Validate(obj); err != nil {
+ return nil, err
+ }
+ }
+
+ key := m.keyFunc(obj)
+ if key == "" {
+ return nil, fmt.Errorf("key is required")
Review comment:
If the error construction is just a plain string, just use `errors.New`.
##########
File path: api/internal/core/store/store_mock_test.go
##########
@@ -0,0 +1,200 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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 store
+
+import (
+ "context"
+ "fmt"
+ "testing"
+
+ "github.com/apisix/manager-api/internal/core/entity"
+ "github.com/apisix/manager-api/internal/utils"
+ "github.com/shiningrush/droplet/data"
+ "github.com/stretchr/testify/assert"
+)
+
+type testObj struct {
+ entity.BaseInfo
+ Name string
+ Value string
+}
+
+func keyFunc(obj interface{}) string {
+ return utils.InterfaceToString(obj.(*testObj).Name)
+}
+
+func TestMock_Create(t *testing.T) {
+ ctx := context.Background()
+ m := NewMock(keyFunc, nil)
+
+ // create success
+ ret, err := m.Create(ctx, &testObj{Name: "test"})
+ assert.Nil(t, err)
+ assert.Equal(t, "test", ret.(*testObj).Name)
+
+ // create failed, "key is required"
+ ret, err = m.Create(ctx, &testObj{Name: ""})
+ assert.Equal(t, fmt.Errorf("key is required"), err)
Review comment:
1. Maybe `assert.Error` method is more proper.
2. Use `errors.New`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] codecov-io edited a comment on pull request #1318: feat: add store mock
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #1318:
URL: https://github.com/apache/apisix-dashboard/pull/1318#issuecomment-761807941
# [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1318?src=pr&el=h1) Report
> Merging [#1318](https://codecov.io/gh/apache/apisix-dashboard/pull/1318?src=pr&el=desc) (f3529f5) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/754f5598706fe5dfe9a09bf0d51d0701406c692c?el=desc) (754f559) will **increase** coverage by `1.82%`.
> The diff coverage is `64.18%`.
[](https://codecov.io/gh/apache/apisix-dashboard/pull/1318?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #1318 +/- ##
==========================================
+ Coverage 44.19% 46.02% +1.82%
==========================================
Files 31 32 +1
Lines 1948 2077 +129
==========================================
+ Hits 861 956 +95
- Misses 975 1002 +27
- Partials 112 119 +7
```
| [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/1318?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [api/internal/core/store/mock\_interface.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1318/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmUvbW9ja19pbnRlcmZhY2UuZ28=) | `32.20% <32.20%> (ø)` | |
| [api/internal/core/store/store\_mock.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1318/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmUvc3RvcmVfbW9jay5nbw==) | `85.39% <85.39%> (+85.39%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1318?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1318?src=pr&el=footer). Last update [754f559...f3529f5](https://codecov.io/gh/apache/apisix-dashboard/pull/1318?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] ShiningRush edited a comment on pull request #1318: feat: add store mock
Posted by GitBox <gi...@apache.org>.
ShiningRush edited a comment on pull request #1318:
URL: https://github.com/apache/apisix-dashboard/pull/1318#issuecomment-761993164
> Original mock doesn't have the store ability, so we need to provide mock data in every unit test.
Yes, this is the job that stub module should do, if you want to reuse stub data, you should just extract those data instead of the logic.
Put the store logic into mock object will cause `Shotgun Surgery` that when we change the logic of store, we need also change the mock object's loigc.
> I think this is more like a fake ETCD and can help we write unit test more convenient.
If you want a fake ETCD, I think we can implement a `MemoryStorage` for `GenericStore`.
However, I must point that this way should just be used by integration test, because **unit test should be isolated from each other**
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] juzhiyuan commented on pull request #1318: feat: add store mock
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on pull request #1318:
URL: https://github.com/apache/apisix-dashboard/pull/1318#issuecomment-766528141
is this urgent?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] codecov-io edited a comment on pull request #1318: feat: add store mock
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #1318:
URL: https://github.com/apache/apisix-dashboard/pull/1318#issuecomment-761807941
# [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1318?src=pr&el=h1) Report
> Merging [#1318](https://codecov.io/gh/apache/apisix-dashboard/pull/1318?src=pr&el=desc) (378bda5) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/754f5598706fe5dfe9a09bf0d51d0701406c692c?el=desc) (754f559) will **increase** coverage by `1.82%`.
> The diff coverage is `64.18%`.
[](https://codecov.io/gh/apache/apisix-dashboard/pull/1318?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #1318 +/- ##
==========================================
+ Coverage 44.19% 46.02% +1.82%
==========================================
Files 31 32 +1
Lines 1948 2077 +129
==========================================
+ Hits 861 956 +95
- Misses 975 1002 +27
- Partials 112 119 +7
```
| [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/1318?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [api/internal/core/store/mock\_interface.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1318/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmUvbW9ja19pbnRlcmZhY2UuZ28=) | `32.20% <32.20%> (ø)` | |
| [api/internal/core/store/store\_mock.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1318/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmUvc3RvcmVfbW9jay5nbw==) | `85.39% <85.39%> (+85.39%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1318?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1318?src=pr&el=footer). Last update [754f559...378bda5](https://codecov.io/gh/apache/apisix-dashboard/pull/1318?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] starsz commented on pull request #1318: feat: add store mock
Posted by GitBox <gi...@apache.org>.
starsz commented on pull request #1318:
URL: https://github.com/apache/apisix-dashboard/pull/1318#issuecomment-761970638
> Why we need this PR? I think the original mock store is enough, please have a look at #859
Original mock doesn't have the store ability, so we need to provide mock data in every unit test.
IMO, I think this is more like a fake ETCD and can help we write unit test more convenient.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] nic-chen commented on pull request #1318: feat: add store mock
Posted by GitBox <gi...@apache.org>.
nic-chen commented on pull request #1318:
URL: https://github.com/apache/apisix-dashboard/pull/1318#issuecomment-762325052
> Yes, this is the job that stub module should do, if you want to reuse stub data, you should just extract those data instead of the logic.
Put the store logic into mock object will cause Shotgun Surgery that when we change the logic of store, we need also change the mock object's logic.
that's what I want to say
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] starsz commented on a change in pull request #1318: feat: add store mock
Posted by GitBox <gi...@apache.org>.
starsz commented on a change in pull request #1318:
URL: https://github.com/apache/apisix-dashboard/pull/1318#discussion_r559278931
##########
File path: api/internal/core/store/store_mock.go
##########
@@ -18,48 +18,184 @@ package store
import (
"context"
+ "encoding/json"
+ "fmt"
+ "sort"
+ "github.com/apisix/manager-api/internal/core/entity"
+ "github.com/shiningrush/droplet/data"
"github.com/stretchr/testify/mock"
)
-type MockInterface struct {
- mock.Mock
+type Mock struct {
+ MockInterface
+ storage map[string]interface{}
+ validator Validator
+ keyFunc func(obj interface{}) string
}
-func (m *MockInterface) Get(key string) (interface{}, error) {
- ret := m.Mock.Called(key)
- return ret.Get(0), ret.Error(1)
+func NewMock(keyFunc func(obj interface{}) string, validator Validator) *Mock {
+ m := &Mock{
+ storage: make(map[string]interface{}),
+ validator: validator,
+ keyFunc: keyFunc,
+ }
+
+ m.onCreate()
+ m.onGet()
+ m.onUpdate()
+ m.onList()
+ m.onBatchDelete()
+ return m
+}
+
+func (m *Mock) create(ctx context.Context, obj interface{}) (interface{}, error) {
+ if setter, ok := obj.(entity.BaseInfoSetter); ok {
+ info := setter.GetBaseInfo()
+ info.Creating()
+ }
+
+ if m.validator != nil {
+ if err := m.validator.Validate(obj); err != nil {
+ return nil, err
+ }
+ }
+
+ key := m.keyFunc(obj)
+ if key == "" {
+ return nil, fmt.Errorf("key is required")
Review comment:
Updated.
##########
File path: api/internal/core/store/store_mock.go
##########
@@ -18,48 +18,184 @@ package store
import (
"context"
+ "encoding/json"
+ "fmt"
+ "sort"
+ "github.com/apisix/manager-api/internal/core/entity"
+ "github.com/shiningrush/droplet/data"
"github.com/stretchr/testify/mock"
)
-type MockInterface struct {
- mock.Mock
+type Mock struct {
+ MockInterface
Review comment:
Updated
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] membphis commented on pull request #1318: feat: add store mock
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #1318:
URL: https://github.com/apache/apisix-dashboard/pull/1318#issuecomment-770788369
ping @starsz
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] starsz commented on pull request #1318: feat: add store mock
Posted by GitBox <gi...@apache.org>.
starsz commented on pull request #1318:
URL: https://github.com/apache/apisix-dashboard/pull/1318#issuecomment-772144347
I will close this PR.
When we need `Integration Testing` then we can reopen it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] starsz commented on a change in pull request #1318: feat: add store mock
Posted by GitBox <gi...@apache.org>.
starsz commented on a change in pull request #1318:
URL: https://github.com/apache/apisix-dashboard/pull/1318#discussion_r559274760
##########
File path: api/internal/core/store/mock_interface.go
##########
@@ -0,0 +1,131 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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 store
+
+import (
+ "context"
+
+ "github.com/stretchr/testify/mock"
+)
+
+type MockInterface struct {
+ mock.Mock
+}
+
+func (m *MockInterface) Get(key string) (interface{}, error) {
+ ret := m.Called(key)
+
+ if rf, ok := ret.Get(0).(func(key string) (interface{}, error)); ok {
Review comment:
Not very agree. Because this file is generated by mockery, and I do little modification.
So maybe we should keep the original structure.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org