Skip to content

feat: test examples#372

Draft
n063h wants to merge 1 commit intomosecorg:mainfrom
n063h:test-examples
Draft

feat: test examples#372
n063h wants to merge 1 commit intomosecorg:mainfrom
n063h:test-examples

Conversation

@n063h
Copy link
Copy Markdown
Contributor

@n063h n063h commented May 29, 2023

ref to #181

Description

Add tests for all runnable examples.

Take the ret_code of client.py as the test result, so a little changes to the existing code are necessary.

Signed-off-by: hang lv <xlv20@fudan.edu.cn>
@github-actions github-actions bot added the enhancement New feature or request label May 29, 2023
Copy link
Copy Markdown

@JiwaniZakir JiwaniZakir left a comment

Choose a reason for hiding this comment

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

In tests/test_examples.py, subprocess.Popen is called without stdout=subprocess.PIPE, stderr=subprocess.PIPE, so when service.communicate() returns on line 78, both stdout and stderr will be None. The failure message assert code == 0, (code, stdout, stderr) will print (code, None, None), providing no useful diagnostic information when a test fails — consider adding those pipe arguments to the Popen call.

The test function is named test_forward_timeout, which is misleading since there's no timeout behavior being tested here; a name like test_example_inference would better reflect what's actually being validated.

In examples/client.py, the condition if k not in ["msgpack"] on line 43 is effectively dead code — "msgpack" is not a key in the req dict, so this branch will never be taken, and the content will always be JSON-encoded. Either the req dict is missing a "msgpack" entry, or this condition should be removed. Additionally, the error message on line 40 says "Please specify a shm to run: plasma or redis" but echo is also a valid key, which will mislead users.

There's also a minor style inconsistency: examples/type_validation/client.py uses the bare exit(1) builtin while the other clients use sys.exit(1)sys.exit is the preferred form in library/script code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants