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

tests(integration): improve test_hello_world #1305

Merged
merged 4 commits into from
Nov 23, 2020

Conversation

bio-howard
Copy link
Contributor

No description provided.

@bio-howard bio-howard requested a review from a team as a code owner November 17, 2020 22:49
@jina-bot jina-bot added size/S area/testing This issue/PR affects testing labels Nov 17, 2020
@nan-wang
Copy link
Member

Bad commit message. Instead of tests(...): ..., test(...): ... is preferred by lint. Please refer to:

@codecov
Copy link

codecov bot commented Nov 19, 2020

Codecov Report

Merging #1305 (9f642a5) into master (e06013d) will increase coverage by 0.69%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1305      +/-   ##
==========================================
+ Coverage   83.14%   83.84%   +0.69%     
==========================================
  Files          95      101       +6     
  Lines        6544     6733     +189     
==========================================
+ Hits         5441     5645     +204     
+ Misses       1103     1088      -15     
Impacted Files Coverage Δ
jina/clients/python/helper.py 85.71% <0.00%> (-9.75%) ⬇️
jina/peapods/remote.py 52.85% <0.00%> (-7.53%) ⬇️
jina/drivers/encode.py 88.88% <0.00%> (-1.12%) ⬇️
jina/docker/hubio.py 69.25% <0.00%> (-0.87%) ⬇️
jina/peapods/jinad.py 64.65% <0.00%> (-0.84%) ⬇️
jina/drivers/multimodal.py 95.34% <0.00%> (-0.81%) ⬇️
jina/drivers/rank.py 96.87% <0.00%> (-0.69%) ⬇️
jina/drivers/search.py 94.64% <0.00%> (-0.67%) ⬇️
jina/logging/profile.py 55.81% <0.00%> (-0.66%) ⬇️
jina/drivers/index.py 95.00% <0.00%> (-0.46%) ⬇️
... and 51 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 d0b2608...9f642a5. Read the comment docs.

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.

I didn't get the point of this test. The original test_helloworld is to check whether jina hello-world can be successfully executed. If the subprocess failed, a CalledProcessError will be thrown.

@bio-howard
Copy link
Contributor Author

@nan-wang as it's described here: #1304

I think that test test_helloworld(tmpdir) doesn't "really" test if we are calling hello-world. The result tells if jina hello-world was called and any option which accepts --workdir parameter was executed properly. It makes a risk that other script will be executed, it won't be hello-world and it will still pass the test.
One cannot assume that is impossible to make a mistake in the script call.

Test which is added by me stops after it will recognize that hello world script was being executed. It's not a big time cost.

@@ -13,10 +13,29 @@


@pytest.mark.timeout(360)
def test_helloworld(tmpdir):
def test_helloworld_call(tmpdir):
Copy link
Contributor

Choose a reason for hiding this comment

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

But then why keep this one as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that it would be better to don't have two asserts in one test, so every test will test other functionality. Now, I don't see any benefit for such approach in this case. I will make one test.

tests/integration/test_helloworld.py Show resolved Hide resolved
@bwanglzu bwanglzu linked an issue Nov 20, 2020 that may be closed by this pull request
@bhavsarpratik
Copy link
Contributor

recheckcla

@github-actions
Copy link

github-actions bot commented Nov 23, 2020

Jina CLA check ✅ All Contributors have signed the CLA.

@bhavsarpratik
Copy link
Contributor

@bio-howard Thanks for the contribution! To merge it you need to sign the CLA as stated above.

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

@bio-howard
Copy link
Contributor Author

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

@bhavsarpratik
Copy link
Contributor

recheckcla

@bhavsarpratik bhavsarpratik merged commit 6559d2d into jina-ai:master Nov 23, 2020
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.

test_helloworld.py::test_helloworld is misleading
6 participants