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(flow): moved test_segmenter.py into a flow unittest #2287

Merged
merged 4 commits into from Apr 17, 2021

Conversation

MiaAltieri
Copy link
Contributor

@MiaAltieri MiaAltieri commented Apr 8, 2021

Flowed the suggestion in #692 to move the tests in test_segmenter.py under flow tests and have the same functionality tested as a simple unittest. Cleaned up some of the duplicated code.

If you'd like to see something else or I've misunderstood please reach out.

@MiaAltieri MiaAltieri requested a review from a team as a code owner April 8, 2021 03:12
@jina-bot jina-bot added size/S area/testing This issue/PR affects testing labels Apr 8, 2021
@codecov
Copy link

codecov bot commented Apr 8, 2021

Codecov Report

Merging #2287 (d847ebe) into master (03b6f21) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2287   +/-   ##
=======================================
  Coverage   90.83%   90.83%           
=======================================
  Files         222      222           
  Lines       11740    11740           
=======================================
  Hits        10664    10664           
  Misses       1076     1076           
Flag Coverage Δ
daemon 51.22% <ø> (ø)
jina 91.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 a87e72c...d847ebe. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Apr 8, 2021

Latency summary

Current PR yields:

  • 😶 index QPS at 1233, delta to last 3 avg.: +0%
  • 😶 query QPS at 20, delta to last 3 avg.: -4%

Breakdown

Version Index QPS Query QPS
current 1233 20
1.1.5 1242 20
1.1.4 1247 20

Backed by latency-tracking. Further commits will update this comment.

Copy link
Member

@nan-wang nan-wang left a comment

Choose a reason for hiding this comment

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

Would you please elaborate on the differences between dummy-seg-not-random.yml and dummy-seg-random.yml?

tests/unit/flow/yaml/dummy-seg-not-random.yml Outdated Show resolved Hide resolved
@nan-wang
Copy link
Member

nan-wang commented Apr 8, 2021

recheckcla

@github-actions
Copy link

github-actions bot commented Apr 8, 2021

Jina CLA check

❤️ Thank you for your pull request. It looks like this is your first contribution to an open source project maintained by Jina AI Limited. Before we can look at your pull request, we kindly ask that you sign our Contributor License Agreement. You can sign it by commenting in format below.


I have read the CLA Document and I hereby sign the CLA


@MiaAltieri
Copy link
Contributor Author

MiaAltieri commented Apr 9, 2021

Thank you for taking your time to review my code. My apologies, while adapting the code from its original source underneath tests/integration/mime I didn't check to see if the files dummy-seg-not-random.yml and dummy-seg-random.yml were in fact different. It appears they are the same, let me update the code to reflect that after my evening meeting. Thank you for your patience

@MiaAltieri
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

nan-wang
nan-wang previously approved these changes Apr 12, 2021
Copy link
Member

@nan-wang nan-wang left a comment

Choose a reason for hiding this comment

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

LGTM👍

@nan-wang
Copy link
Member

@MiaAltieri Thanks for your contribution. However this PR does not pass our docstring and black checks. Please make sure you have the precommit properly set up locally. If you still have the same issue, you will need to either rebase your PR or create a new one.

reference:
https://github.com/jina-ai/jina/blob/master/CONTRIBUTING.md#-making-your-first-submission

@MiaAltieri
Copy link
Contributor Author

MiaAltieri commented Apr 14, 2021

Hi I apologize for committing again without it passing both the docstring check and black. I thought I had done it correctly since black said it passed by reporting the following after I committed:
black....................................................................Passed
Let me look into this more.

If you have any suggestions for things I could check let me know. I noticed that when I looked into the tests that failed on both black and check-dockstring (on git) they both say "Error: The head commit for this pull_request event is not ahead of the base commit. Please submit an issue on this action's GitHub repo." however when I looked at my forked repository it was up to date.

I can try rebasing/creating a new one

@JoanFM
Copy link
Member

JoanFM commented Apr 14, 2021

Hi I apologize for committing again without it passing both the docstring check and black. I thought I had done it correctly since black said it passed by reporting the following after I committed:
black....................................................................Passed
Let me look into this more.

If you have any suggestions for things I could check let me know. I noticed that when I looked into the tests that failed on both black and check-dockstring (on git) they both say "Error: The head commit for this pull_request event is not ahead of the base commit. Please submit an issue on this action's GitHub repo." however when I looked at my forked repository it was up to date.

I can try rebasing/creating a new one

Hey @MiaAltieri , merging with master should be good enough

Co-authored-by: Nan Wang <nan.wang@jina.ai>
@MiaAltieri
Copy link
Contributor Author

MiaAltieri commented Apr 15, 2021

Thanks for the tip, rebased into one commit and merged with master

@hanxiao hanxiao merged commit dbe5a59 into jina-ai:master Apr 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing This issue/PR affects testing size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants