Skip to content

Improve unit tests - #985

Merged
mfeurer merged 12 commits into
developfrom
randomize_test_order
Nov 3, 2020
Merged

Improve unit tests#985
mfeurer merged 12 commits into
developfrom
randomize_test_order

Conversation

@mfeurer

@mfeurermfeurer commented Oct 30, 2020•
edited
Loading

Copy link
Copy Markdown
Collaborator
  • increase wait time when uploading a run to 600s. It's better to wait a very long time instead of failing that test (on CI)
  • reduce the warnings a bit to have less output online
  • remove deprecated format argument to OpenMLDataset
  • add load balancing for parallel unit tests

@mfeurermfeurer changed the title Randomize test orderImprove unit testsOct 30, 2020
@mfeurer
mfeurerforce-pushed the randomize_test_order branch from ccec20d to 60bd37aCompareNovember 2, 2020 09:16
@codecov-io

codecov-io commented Nov 2, 2020•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #985 into develop will increase coverage by 0.07%.
The diff coverage is 83.33%.

Impacted file tree graph

@@ Coverage Diff @@## develop #985 +/- ##
===========================================
+ Coverage 87.85% 87.93% +0.07% 
===========================================
Files 36 36 Lines 4555 4551 -4 ===========================================
Hits 4002 4002 + Misses 553 549 -4
Impacted FilesCoverage Δ
openml/flows/flow.py92.71% <0.00%> (ø)
openml/datasets/dataset.py83.33% <100.00%> (+0.55%)⬆️
openml/extensions/sklearn/extension.py91.08% <100.00%> (ø)
openml/study/functions.py88.23% <100.00%> (ø)
openml/runs/functions.py83.58% <0.00%> (+0.25%)⬆️

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 63ec0ae...39e2f07. Read the comment docs.

PGijsbers
PGijsbers requested changes Nov 2, 2020
View reviewed changes

@PGijsbersPGijsbers left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Overall looks good, just would like some minor clarifications.

Comment threadtests/test_runs/test_run_functions.py Outdated
n_missing_vals = self.TEST_SERVER_TASK_SIMPLE[1]
n_test_obs = self.TEST_SERVER_TASK_SIMPLE[2]
self._run_and_upload_classification(lr, task_id, n_missing_vals, n_test_obs, "62501")
self.assertLessEqual(warn_mock.call_count, 3)

@PGijsbersPGijsbersNov 2, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please explain why we expect three warnings.

@mfeurermfeurerNov 2, 2020

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Honestly, I don't understand the 3 myself and would have expected 1. I will simply increase the max_iter so this test passes.

@mfeurermfeurerNov 2, 2020

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Ah, I now understand. We call run_model_on_task once, and then twice a helper function called _rerun_model_and_compare_predictions. Anyway, I think increasing the number iterations is a bit cleaner.

Comment threadtests/test_runs/test_run_functions.py
Comment threadtests/test_runs/test_run_functions.py
PGijsbers
PGijsbers requested changes Nov 2, 2020
View reviewed changes

@PGijsbersPGijsbers left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good overall, just expecting some minor explanations.

PGijsbers
PGijsbers reviewed Nov 2, 2020
View reviewed changes
Comment threadtests/test_datasets/test_dataset_functions.py
PGijsbers
PGijsbers approved these changes Nov 2, 2020
View reviewed changes

@PGijsbersPGijsbers left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM 👍

@mfeurer
mfeurer merged commit a629562 into developNov 3, 2020
@mfeurer
mfeurer deleted the randomize_test_order branch November 3, 2020 07:35
github-actionsBot pushed a commit that referenced this pull request Nov 3, 2020
Github Actions
Matthias Feurer: Improve unit tests (#985)
6612f86
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PGijsbersPGijsbersPGijsbers approved these changes
@sahithyaravisahithyaraviAwaiting requested review from sahithyaravi

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mfeurer@codecov-io@PGijsbers