Skip to content
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

Java e2e document controller #161

Merged
merged 10 commits into from
Jun 18, 2018
Merged

Java e2e document controller #161

merged 10 commits into from
Jun 18, 2018

Conversation

jenow
Copy link
Contributor

@jenow jenow commented Jun 13, 2018

What does this PR do ?

e2e tests for JAVA document controller

How should this be manually tested?

cd internal/wrappers/features/java
gradle cucumber

Other changes

Bug fixes

@codecov-io
Copy link

codecov-io commented Jun 13, 2018

Codecov Report

Merging #161 into 1.x will decrease coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              1.x     #161      +/-   ##
==========================================
- Coverage   89.01%   88.92%   -0.09%     
==========================================
  Files         244      244              
  Lines        4406     4407       +1     
==========================================
- Hits         3922     3919       -3     
- Misses        452      456       +4     
  Partials       32       32
Impacted Files Coverage Δ
document/mDelete.go 100% <100%> (ø) ⬆️
document/get.go 100% <100%> (ø) ⬆️
document/search.go 100% <100%> (ø) ⬆️
document/count.go 92% <100%> (+0.69%) ⬆️
realtime/subscribe.go 71.42% <0%> (-5.72%) ⬇️
realtime/join.go 72.97% <0%> (-5.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25f8121...a2086fe. Read the comment docs.

@jenow jenow changed the title Java e2e document Java e2e document controller Jun 13, 2018
@jenow jenow mentioned this pull request Jun 13, 2018
Copy link
Member

@etrousset etrousset left a comment

Choose a reason for hiding this comment

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

As I commented on other PR, steps related to m* API could be written using lists of entries, so that it is more obvious the step is working using a list as input e.g [ 'my-fist-doc', 'my-other-doc', ... ]

goStrings := make([]string, len)
for i, s := range tmpslice {
goStrings[i] = C.GoString(s)
goStrings := make([]string, 0)
Copy link
Contributor

@scottinet scottinet Jun 18, 2018

Choose a reason for hiding this comment

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

What was wrong with the previous version of that function?

Also, since we know the slice's capacity, we may as well use that information to prevent regrowing the slice possibly a few times during the copy: goStrings := make([]string, 0, len)

Copy link
Contributor Author

@jenow jenow Jun 18, 2018

Choose a reason for hiding this comment

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

Nothing, we do a append in every other function so I thought it would be good to also do append in this function, only for consistency.
Good catch for the len though

@@ -116,7 +118,9 @@ func kuzzle_document_validate(d *C.document, index *C.char, collection *C.char,
//export kuzzle_document_search
func kuzzle_document_search(d *C.document, index *C.char, collection *C.char, body *C.char, options *C.query_options) *C.search_result {
res, err := (*document.Document)(d.instance).Search(C.GoString(index), C.GoString(collection), json.RawMessage(C.GoString(body)), SetQueryOptions(options))
return goToCSearchResult(res, err)
r := goToCSearchResult(res, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the added value of storing the result in an intermediary variable before returning it?

And the collection has a document with id 'mupdate-my-document-id'
And the collection has a document with id 'mupdate-my-document-id2'
When I update the documents ['unknown', 'mupdate-my-document-id2']
And the document 'mupdate-my-document-id2' should be updated
Copy link
Member

Choose a reason for hiding this comment

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

This line should be a Then.

And it has a collection 'exist-test-collection'
And the collection has a document with id 'exist-my-document-id'
When I check if 'exist-my-document-unknown' exists
Then the document should not exists
Copy link
Member

Choose a reason for hiding this comment

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

"exist" instead of "exists" has this is an adjective

And it has a collection 'mget-test-collection'
And the collection has a document with id 'mget-my-document-id'
And the collection has a document with id 'mget-my-document-id2'
When I get document ['mget-my-document-id', 'mget-my-document-id2']
Copy link
Member

Choose a reason for hiding this comment

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

"documentS"

@jenow jenow merged commit 3568476 into 1.x Jun 18, 2018
@jenow jenow deleted the java-e2e-document branch June 18, 2018 13:50
@jenow jenow mentioned this pull request Sep 10, 2018
@jenow jenow mentioned this pull request Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants