-
-
Notifications
You must be signed in to change notification settings - Fork 211
Fix unit tests #448
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
Fix unit tests #448
Conversation
| for did in datasets: | ||
| self._check_dataset(datasets[did]) | ||
|
|
||
| # TODO implement these 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.
Sure you didn't forget these?
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.
@janvanrijn you mean we should not verify the datasets?
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 mean there's still a TODO in the PR ;)
(line 16)
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.
Ah you are right, I did not see what you meant because of the lines selected. The TODO seems already implemented.
| downloaded_flow = openml.flows.get_flow(flow_exists) | ||
| setup_exists = openml.setups.setup_exists(downloaded_flow) | ||
| self.assertIsInstance(setup_exists, int) | ||
| self.assertGreater(setup_exists, 0) |
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.
Maybe we can just use assertGreater instead of also making use of assertIsInstance, since if the argument is False it will throw AttributeError: 'NoneType' object has no attribute 'fail'.
This PR fixes a few unit tests and adds one which I forgot to check in quite a while ago. Most importantly, the return values to retrieve all runs for a combination of task/setup is now consistent. Furthermore, I think cleaning the test server from lots of runs was really important.