Making unit tests green by Neeratyoy · Pull Request #748 · openml/openml-python

Neeratyoy · 2019-07-24T16:23:45Z

What does this PR implement/fix? Explain your changes.

Owing to the concurrent runs, combined with deletion of files at the end of unit test modules, some tests may fail. Rerunning them should ensure they pass.

codecov-io · 2019-07-25T10:47:37Z

Codecov Report

Merging #748 into develop will decrease coverage by 0.27%.
The diff coverage is 100%.

@@ Coverage Diff @@## develop #748 +/- ##
===========================================
- Coverage 88.06% 87.79% -0.28% 
===========================================
Files 36 36 Lines 4090 4096 +6 ===========================================
- Hits 3602 3596 -6 - Misses 488 500 +12

Impacted Files	Coverage Δ
openml/testing.py	`89.06% <100%> (-6.24%)`	⬇️
openml/config.py	`89.87% <0%> (-2.54%)`	⬇️
openml/_api_calls.py	`87.01% <0%> (-1.3%)`	⬇️
openml/runs/functions.py	`82.07% <0%> (-0.29%)`	⬇️
openml/evaluations/functions.py	`92.17% <0%> (+2.62%)`	⬆️

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 e6ee09d...d635500. Read the comment docs.

Neeratyoy · 2019-08-01T15:34:30Z

@mfeurer, @PGijsbers
If I would like to import a new package for the test cases, isn't it enough to add to ci_scripts/install.shhere.

It looks like the appveyor build failed to install it. How can I fix that?

mfeurer · 2019-08-02T07:51:48Z

Appveyor doesn't use install.sh. However, I doubt that posix-ipc will work on Windows, see for example the description of the project here. Probably we should discuss this offline on Monday.

PGijsbers · 2019-08-05T07:31:47Z

I hope you can figure something out. But to answer the original question, if you want to add test dependencies, it's best to add it in setup.py.

Neeratyoy · 2019-08-05T18:32:10Z

@mfeurer
if we delete all the files for each of the workers independently and asynchronously, test_list_datasets_with_high_size_parameter fails.

mfeurer · 2019-08-06T07:17:09Z

That's unfortunate. Maybe we should execute that test on the live server?

Neeratyoy · 2019-08-06T12:10:46Z

That's unfortunate. Maybe we should execute that test on the live server?

I guess you mean a new instance of an OpenML server every test run?
Will that interfere with the fetch requests to the test server, that is, by having a new openml.config.server value.

mfeurer · 2019-08-06T12:21:58Z

Sorry, I don't get that. What I meant is like https://github.com/openml/openml-python/blob/develop/tests/test_datasets/test_dataset_functions.py#L213

Neeratyoy · 2019-08-06T12:29:19Z

aah you are okay with the dataset list being fetched from the prod server
alright, I was thinking we shouldn't be touching that during test but this is a read-only operation
makes sense
thanks

PGijsbers · 2019-08-06T15:10:35Z

We do it at several points in our unit tests. But your general intuition is right, only do it if you have to. In particular never alter the production server and think twice if you really need the production server if it's a demanding query. In this case for the reasons you pointed out it makes sense.

Neeratyoy · 2019-08-07T11:40:36Z

@mfeurer
all passed, except one unit test that gave:
openml.exceptions.OpenMLServerException: An Elastic Search Exception occured

Neeratyoy · 2019-08-07T12:01:46Z

Thanks!

mfeurer

I like this, this is much simpler now. I left a few comments on how I think this could be improved.

mfeurer · 2019-08-07T12:19:40Z

+ logger = logging.getLogger("unit_tests_publish")
+ logger.setLevel(logging.DEBUG)
+ fh = logging.FileHandler('UploadedFiles.log')
+# fh.setLevel(logging.DEBUG)


Is it actually necessary to introduce a logger which logs to a file? Also, could you please remove that comment?

mfeurer · 2019-08-07T12:21:43Z

+from openml.testing import TestBase
+
+# creating logger for unit test file deletion status
+logger = logging.getLogger("unit_tests")


This looks a lot like a copy of the logger above. Also, I assume that the logger names should be swapped.

mfeurer · 2019-08-07T12:22:00Z

+# creating logger for unit test file deletion status
+logger = logging.getLogger("unit_tests")
+logger.setLevel(logging.DEBUG)
+fh = logging.FileHandler('UnitTestDeletion.log')


Same as above, do we really need to save this to disk?

I agree, no.
I was tailing during the testing and this helped.
Will have them removed.

mfeurer · 2019-08-07T12:24:57Z

+ openml.config.server = TestBase.test_server
+ openml.config.apikey = TestBase.apikey
+
+ # legal_entities defined in openml.utils._delete_entity() - {'user'}


Are these comments still needed?

Neeratyoy added 2 commits July 24, 2019 18:21

Adding flaky rerun decorators for stochastic failures
92864e8

Increasing number of repeats for stochastic failures
88f9874

Neeratyoy added 3 commits July 25, 2019 14:32

Increasing retries; Fixing PEP8
dfc4d1a

Small update to logging behaviour for unit testing
12ab228

Increasing retries till it works
54ad6a0

mfeurer reviewed Jul 26, 2019
View reviewed changes

Comment threadtests/test_datasets/test_dataset_functions.py Outdated

Neeratyoy added 2 commits July 29, 2019 16:11

Fixing unit test waiting for server processing
7a551fb

Revamping deletion of files after unit tests
c452acd

Adding comments/descriptions
02f867c

Neeratyoy marked this pull request as ready for review August 1, 2019 17:19

Neeratyoy changed the title ~~Adding flaky rerun decorators for stochastic failures of unit tests~~Making unit tests greenAug 1, 2019

Neeratyoy added 5 commits August 1, 2019 19:20

Merge branch 'develop' into fix_unit_test_list_dataset
421bbff

Debugging
f35c2de

Debugging
4447114

Fixing directory issue for test cases
d02da76

Doubling wait time for test_run_and_upload_gridsearch
b92d3e3

Neeratyoy added 4 commits August 5, 2019 15:13

Removing semaphore implementation
968883f

Fixing path issue for appveyor tests
de2bb08

Debugging appveyor path
50c0491

Fixing PEP8
2915435

Fixing test_list_datasets_with_high_size_parameter
c500cad

PEP8 fix
3c9dfb6

Neeratyoy requested a review from mfeurer August 7, 2019 11:40

mfeurer reviewed Aug 7, 2019
View reviewed changes

Removing logging to disk
d635500

Neeratyoy mentioned this pull request Aug 7, 2019
Clean test server of all files uploaded during unit tests #757
Open

mfeurer approved these changes Aug 7, 2019
View reviewed changes

mfeurer merged commit 4b84dc6 into developAug 7, 2019

mfeurer deleted the fix_unit_test_list_dataset branch August 7, 2019 14:33

Uh oh!

Uh oh!

Conversation

Neeratyoy commented Jul 24, 2019

What does this PR implement/fix? Explain your changes.

Uh oh!

codecov-io commented Jul 25, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Neeratyoy commented Aug 1, 2019

Uh oh!

mfeurer commented Aug 2, 2019

Uh oh!

PGijsbers commented Aug 5, 2019

Uh oh!

Neeratyoy commented Aug 5, 2019

Uh oh!

mfeurer commented Aug 6, 2019

Uh oh!

Neeratyoy commented Aug 6, 2019

Uh oh!

mfeurer commented Aug 6, 2019

Uh oh!

Neeratyoy commented Aug 6, 2019

Uh oh!

PGijsbers commented Aug 6, 2019

Uh oh!

Neeratyoy commented Aug 7, 2019

Uh oh!

Neeratyoy commented Aug 7, 2019

Uh oh!

mfeurer left a comment

Choose a reason for hiding this comment

Uh oh!

mfeurerAug 7, 2019

Choose a reason for hiding this comment

Uh oh!

mfeurerAug 7, 2019

Choose a reason for hiding this comment

Uh oh!

mfeurerAug 7, 2019

Choose a reason for hiding this comment

Uh oh!

NeeratyoyAug 7, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mfeurerAug 7, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Jul 25, 2019•
edited
Loading