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

test: replace all random docs with new api #1356

Merged
merged 8 commits into from
Nov 27, 2020
Merged

Conversation

bwanglzu
Copy link
Member

@bwanglzu bwanglzu commented Nov 25, 2020

  1. Replace all random_docs with random_docs_new_api, and raname random_docs_new_api to random_docs. (didn't turn it into a conftest fixture since it has a lot of parameters)
  2. Add workspace fixture to some test cases, ensure stored in tmpdir.
  3. Complete & refactor DocumentSet test. During refactoring, fixed some bugs in DocumentSet.

What has been fixed in DocumentSet:

  1. __setitem__ calls CopyFrom, which expect to receive a Document, passed Document.as_pb_object.
  2. __setitem__ for string object not setting, but returning.
  3. __getitem__ when isinstance(key, str), we convert the value into a string.

@jina-bot jina-bot added size/M area/testing This issue/PR affects testing labels Nov 25, 2020
@bwanglzu bwanglzu self-assigned this Nov 25, 2020
@jina-bot jina-bot added area/core This issue/PR affects the core codebase component/type labels Nov 26, 2020
@bwanglzu bwanglzu marked this pull request as ready for review November 26, 2020 23:49
@bwanglzu bwanglzu requested a review from a team as a code owner November 26, 2020 23:49
@codecov
Copy link

codecov bot commented Nov 27, 2020

Codecov Report

Merging #1356 (79ad6da) into master (086b21e) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1356      +/-   ##
==========================================
+ Coverage   82.95%   83.01%   +0.05%     
==========================================
  Files         103      103              
  Lines        6746     6746              
==========================================
+ Hits         5596     5600       +4     
+ Misses       1150     1146       -4     
Impacted Files Coverage Δ
jina/types/sets/document_set.py 98.24% <100.00%> (+15.78%) ⬆️
jina/peapods/grpc_asyncio.py 75.51% <0.00%> (-5.11%) ⬇️
jina/logging/sse.py 91.93% <0.00%> (-3.23%) ⬇️
jina/peapods/gateway.py 94.32% <0.00%> (-0.71%) ⬇️
jina/types/document/__init__.py 97.30% <0.00%> (+0.44%) ⬆️
jina/peapods/pea.py 92.44% <0.00%> (+0.71%) ⬆️

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 086b21e...4d3651f. Read the comment docs.

@@ -3,7 +3,7 @@ with:
index_filename: tmp2
metas:
name: test2
workspace: $JINA_TEST_INDEX
workspace: $TEST_WORKDIR
Copy link
Member

Choose a reason for hiding this comment

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

We arr trying to set the habit to prefix env variablws with JINA so please keep it as it was

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll change it back

@@ -49,7 +49,7 @@ def __getitem__(self, item):
if isinstance(item, int):
return Document(self._docs_proto[item])
elif isinstance(item, str):
return Document(self._docs_map[str(item)])
Copy link
Member

Choose a reason for hiding this comment

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

this is required, as UniqueId can not be directly used as a string

Copy link
Member

Choose a reason for hiding this comment

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

oh but the test doesn't complain 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

image
@hanxiao this is what I got in terminal

@hanxiao hanxiao merged commit 0098cad into master Nov 27, 2020
@hanxiao hanxiao deleted the test-replace-random-docs branch November 27, 2020 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core This issue/PR affects the core codebase area/testing This issue/PR affects testing component/type size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants