Skip to content

Simplify and deduplicate test code #642

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.

Sign up for GitHub

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

Jump to bottom
Merged
desilinguist merged 8 commits into from
Nov 13, 2020
Merged

Simplify and deduplicate test code #642

desilinguist merged 8 commits into from
Nov 13, 2020

Conversation

@mulhod
Copy link
Contributor

@mulhod mulhod commented Nov 12, 2020 •
edited by desilinguist
Loading

Closes #635.

@mulhod mulhod requested review from a user and desilinguist November 13, 2020 00:24
@pep8speaks
Copy link

pep8speaks commented Nov 13, 2020 •
edited
Loading

Hello @mulhod! Thanks for updating this PR.

Line 721:101: E501 line too long (102 > 100 characters)

Line 1148:101: E501 line too long (102 > 100 characters)

Comment last updated at 2020-11-13 17:39:06 UTC

@mulhod mulhod changed the title Simplify and eduplicate test code Simplify and deduplicate test code Nov 13, 2020
hlepp
hlepp reviewed Nov 13, 2020
View reviewed changes
tests/test_ablation.py

import csv
import json
import os
Copy link

@hlepp hlepp Nov 13, 2020

Choose a reason for hiding this comment

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

Why remove import?

Copy link
Contributor Author

@mulhod mulhod Nov 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It was only being used for os.unlink, which isn't being used anymore. Now pathlib.Path.unlink is being used (but it's actually used indirectly -- it's in a helper method in tests.utils that coerces the argument to a Path so that we don't have to care about whether the path is a str or a Path).

@codecov
Copy link

codecov bot commented Nov 13, 2020 •
edited
Loading

Codecov Report

Merging #642 (2daaa24) into main (3dafc5f) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #642   +/-   ##
=======================================
  Coverage   95.11%   95.11%           
=======================================
  Files          27       27           
  Lines        3089     3089           
=======================================
  Hits         2938     2938           
  Misses        151      151
Impacted Files Coverage Δ
skll/data/readers.py 90.13% <100.00%> (ø)

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 3dafc5f...2daaa24. Read the comment docs.

@mulhod mulhod assigned mulhod and unassigned mulhod Nov 13, 2020
@mulhod
Copy link
Contributor Author

mulhod commented Nov 13, 2020

This PR removes over 450 lines from the tests!

desilinguist
desilinguist requested changes Nov 13, 2020
View reviewed changes
Copy link
Collaborator

@desilinguist desilinguist left a comment

Choose a reason for hiding this comment

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

This is fantastic, @mulhod! Thanks for taking this on – this is a really nice improvement of the codebase.

I made a couple of minor suggestions. I also have a couple of broader suggestions:

  1. Can you run the tests locally to make sure that all the created files are actually being cleaned up properly? As long as we are working on this, it would be good to make sure of those.

  2. I am a little concerned about changing the from tests.utils import X statements to from .utils import X in terms of the impact this will have on the testing of the PyPi and conda packages. In those scenarios, we install the conda package, do a sparse checkout of the skll tests and then run the tests. I think you should be able to test this by making a local conda package and simulating this scenario.

Thanks again!

@mulhod
Copy link
Contributor Author

mulhod commented Nov 13, 2020

This is fantastic, @mulhod! Thanks for taking this on – this is a really nice improvement of the codebase.

I made a couple of minor suggestions. I also have a couple of broader suggestions:

  1. Can you run the tests locally to make sure that all the created files are actually being cleaned up properly? As long as we are working on this, it would be good to make sure of those.
  2. I am a little concerned about changing the from tests.utils import X statements to from .utils import X in terms of the impact this will have on the testing of the PyPi and conda packages. In those scenarios, we install the conda package, do a sparse checkout of the skll tests and then run the tests. I think you should be able to test this by making a local conda package and simulating this scenario.

Thanks again!

Thanks, @desilinguist! I decided to revert the parts that used relative imports for now. It's a minor change, anyway. I have run all the tests and fixed a couple cases (one that you pointed out!). I think most of the cases were actually issues with the tests that didn't have anything to do with my changes. But, this gave me the chance to really see what files are left behind, so I just fixed any I saw.

@mulhod mulhod requested review from desilinguist and hlepp November 13, 2020 17:42
@desilinguist
Copy link
Collaborator

desilinguist commented Nov 13, 2020

Thanks so much for doing that @mulhod !

desilinguist
desilinguist approved these changes Nov 13, 2020
View reviewed changes
Copy link
Collaborator

@desilinguist desilinguist left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@desilinguist desilinguist merged commit 91f4d19 into main Nov 13, 2020
@delete-merged-branch delete-merged-branch bot deleted the feature/reduce_code_duplication_in_tests branch November 13, 2020 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@desilinguist desilinguist desilinguist approved these changes

@aloukina aloukina Awaiting requested review from aloukina

@aoifecahill aoifecahill Awaiting requested review from aoifecahill

@cml54 cml54 Awaiting requested review from cml54

@farigys farigys Awaiting requested review from farigys

@jimmybru jimmybru Awaiting requested review from jimmybru

@rutujaubale rutujaubale Awaiting requested review from rutujaubale

@hlepp hlepp Awaiting requested review from hlepp

+1 more reviewer
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Reduce code duplication in tests

5 participants

@mulhod @pep8speaks @desilinguist @hlepp