Skip to content

Fix duplicate key test case #111

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

Merged
merged 2 commits into from
Nov 26, 2021
Merged

Fix duplicate key test case #111

merged 2 commits into from
Nov 26, 2021

Conversation

DifferentialOrange
Copy link
Member

Issue is described in #105. After further investigation, it was found that it was meant to be "insert structure using space name" test case that must process with success instead of "duplicate key error". Test case and corresponding output was changed to be consistent in first commit.
Before patch test case:

// insert new tuple { 11, 1 }
resp, err = client.Insert("test", &Tuple{Id: 10, Msg: "test", Name: "one"})
fmt.Println("Insert Error", err)
fmt.Println("Insert Code", resp.Code)
fmt.Println("Insert Data", resp.Data)

Since there is a duplicate key error test case in another file, coverage has not decreased.

resp, err = conn.Insert(spaceNo, &Tuple{Id: 1, Msg: "hello", Name: "world"})
if tntErr, ok := err.(Error); !ok || tntErr.Code != ErrTupleFound {
t.Errorf("Expected ErrTupleFound but got: %v", err)
}
if len(resp.Data) != 0 {
t.Errorf("Response Body len != 1")
}

Closes #105

Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

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

Seems meaningful for me (but I don't know this code in fact).

Copy link

@LeonidVas LeonidVas left a comment

Choose a reason for hiding this comment

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

Hi! Thank you for the patch.
See several commits below:

  • I think that "Make test case consistent with comments" is more a workaround rather than a fix. Because we have lost "Duplicate key ..." test case. But it seems ok to me. In fact, using an error message to check an error is bad practice, an error code should be used for this purpose.
  • Add a description of a problem to a commit message (something like this in the issue description). This allows to understand the reason of changes in the future.

@DifferentialOrange
Copy link
Member Author

* I think that "Make test case consistent with comments" is more a workaround rather than a fix. Because we have lost "Duplicate key ..." test case. But it seems ok to me. In fact, using an error message to check an error is bad practice, an error code should be used for this purpose.

It may seems like this, so at first I wanted to add a separate test case for Duplicate error. But such test case already is covered by tarantool_test.lua with error code assert. See more in PR description.

@LeonidVas
Copy link

* I think that "Make test case consistent with comments" is more a workaround rather than a fix. Because we have lost "Duplicate key ..." test case. But it seems ok to me. In fact, using an error message to check an error is bad practice, an error code should be used for this purpose.

It may seems like this, so at first I wanted to add a separate test case for Duplicate error. But such test case already is covered by tarantool_test.lua with error code assert. See more in PR description.

Ouch! I missed it. OK.

Original test case had failed with Tarantool 2.8.1 or newer due to error
message rework [1]. Based on code comments and @funny-falcon response
in #105, the test has been planned to be success insert test and not
a duplicate key error test. This patch changes test case and asserts
to its original idea. Since duplicate key test already exists
in tarantool_test.go file [2], coverage should not decrease.

1. tarantool/tarantool@d11fb30
2. https://github.com/tarantool/go-tarantool/blob/61f3a41907b6bcb060e9fa07069cde5b33ba9764/tarantool_test.go#L437-L443

Closes #105
@DifferentialOrange
Copy link
Member Author

* Add a description of a problem to a commit message (something like this in the issue description). This allows to understand the reason of changes in the future.

Added a description and duplicated PR commets to commits.

@Totktonada
Copy link
Member

@funny-falcon Aren't you mind to glance on these changes?

@funny-falcon funny-falcon merged commit 706ffcd into master Nov 26, 2021
@DifferentialOrange DifferentialOrange deleted the 105-fix-tests branch January 13, 2022 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test fails with Tarantool 2.8.1 or newer
4 participants