-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Check output in example tests #7280
Conversation
a973930
to
adce6d0
Compare
I found pytest-repeat useful to investigate the acceptable fluctuations of the statistics, but it does not support |
c=(float, lambda x: 0.62 < x < 0.82), | ||
d=(float, lambda x: 0.83 < x < 0.98), | ||
e=(float, lambda x: 0. < x < 100.), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to find information for choosing this new format method.
- Possible other solutions
- Pros and cons of these solutions (and thus why the above code is good)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I didn't come up with any other reasonable solutions.
I'll try to list possibilities any way.
-
Write in each test a logic to check the output line by line
- Pros: What's being done in each step is clear from test implementation.
- Cons: Prohibitively difficult to write/maintain test cases.
-
Use a regular expression to capture the numbers, and use lambdas to check the numerical ranges.
- Pros: Being a standard notation
- Cons: Difficult to express the output with a regular expression. It's also not easy to interpret for human. Possibilities to not be able to express with the syntax.
-
This method
- Pros: Easy to interpret test cases. Easy to add a new test case (just copy-paste the output and replace some parts with
{...}
). Flexible (any checks can be added later). - Cons: Occasionally new logic might be needed in the tool behind. Being non-standard.
- Pros: Easy to interpret test cases. Easy to add a new test case (just copy-paste the output and replace some parts with
This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 30 days if no further activity occurs. Thank you for your contributions. |
This issue is closed as announced. Feel free to re-open it if needed. |
ecc7a2b
to
bb48251
Compare
Rebased as #7475 has been merged. |
bb48251
to
ffc12c4
Compare
PTAL |
490ade4
to
f9402c5
Compare
Conflict resolved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Seems like we need to install matplotlib
on Travis, or we could either remove the dependence from the example test?
matplotlib cannot be removed from requirements. I add a step to install matplotlib in Travis CI. |
scripts/ci/travis/run-tests.sh
Outdated
@@ -174,6 +174,7 @@ case "${CHAINER_TRAVIS_TEST}" in | |||
install) | |||
run_prestep install_chainer_test_deps | |||
run_prestep chainer_install_from_sdist | |||
run_prestep chainer_example_tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't be install_chainer_example_tests
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops... Force-pushed in f586e60.
Thanks!
ef4ea45
to
f586e60
Compare
Jenkins, test this please |
Jenkins CI test (for commit f586e60, target branch master) succeeded! |
Fixes #6094
Depends on #7277