8000 fix(core): add complex test for routing list by jacobowitz · Pull Request #2760 · jina-ai/serve · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix(core): add complex test for routing list #2760

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 1 commit into from
Jul 7, 2021

Conversation

jacobowitz
Copy link
Contributor
@jacobowitz jacobowitz commented Jun 24, 2021

This adds a test to verify that #2500 got fixed as a side effect of dynamic routing (#2551).
Not sure we need this test actually as part of the test suite, but I was writing it anyway to verify the behaviour and I think there are no other tests doing this.

┆Issue is synchronized with this Asana task by Unito

@jacobowitz jacobowitz requested a review from a team as a code owner June 24, 2021 13:15
@jacobowitz jacobowitz requested review from davidbp and deepankarm June 24, 2021 13:15
@jina-bot jina-bot added size/S area/testing This issue/PR affects testing labels Jun 24, 2021
@codecov
Copy link
codecov bot commented Jun 24, 2021

Codecov Report

Merging #2760 (3605823) into master (0fd44bf) will increase coverage by 0.90%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2760      +/-   ##
==========================================
+ Coverage   87.12%   88.03%   +0.90%     
==========================================
  Files         138      138              
  Lines        9377     9377              
==========================================
+ Hits         8170     8255      +85     
+ Misses       1207     1122      -85     
Flag Coverage Δ
daemon 43.29% <ø> (+0.02%) ⬆️
jina 87.98% <ø> (+0.90%) ⬆️

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

Impacted Files Coverage Δ
jina/peapods/runtimes/gateway/http/models.py 100.00% <0.00%> (+0.74%) ⬆️
jina/types/routing/table.py 93.39% <0.00%> (+0.94%) ⬆️
jina/peapods/zmq/__init__.py 86.02% <0.00%> (+1.92%) ⬆️
jina/helper.py 79.57% <0.00%> (+1.94%) ⬆️
jina/peapods/pods/__init__.py 86.50% <0.00%> (+3.68%) ⬆️
jina/peapods/peas/helper.py 92.68% <0.00%> (+4.87%) ⬆️
jina/peapods/pods/compound.py 93.33% <0.00%> (+6.66%) ⬆️
jina/peapods/peas/__init__.py 98.11% <0.00%> (+6.91%) ⬆️
jina/peapods/runtimes/jinad/client.py 84.02% <0.00%> (+7.10%) ⬆️
jina/peapods/networking.py 100.00% <0.00%> (+8.57%) ⬆️
... and 2 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 0fd44bf...3605823. Read the comment docs.

@hanxiao
Copy link
Member
hanxiao commented Jun 25, 2021

@jacobowitz jacobowitz force-pushed the fix-core-routes-list branch from d65b657 to 3605823 Compare July 7, 2021 08:58
Copy link
Member
@bwanglzu bwanglzu left a comment

Choose a reason for hiding this comment

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

can you benchmark these two test if slow mark it as pytest.mark.slow?

@jacobowitz
Copy link
Contributor Author

can you benchmark these two test if slow mark it as pytest.mark.slow?

I checked its 3 seconds. Lets not mark it as slow as its rather nice to execute locally (no docker etc involved)

Copy link
Member
@bwanglzu bwanglzu left a comment

Choose a reason for hiding this comment

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

LGTM!

@jacobowitz jacobowitz merged commit f43ac77 into master Jul 7, 2021
@jacobowitz jacobowitz deleted the fix-core-routes-list branch July 7, 2021 11:59
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
De 3B2C velopment

Successfully merging this pull request may close these issues.

4 participants
0