Skip to content
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

Add unit tests #27

Merged
merged 5 commits into from
Apr 19, 2020
Merged

Add unit tests #27

merged 5 commits into from
Apr 19, 2020

Conversation

NaleRaphael
Copy link
Contributor

Hi @davidtvs, this is a draft of unit tests for this package. Let's continue the discussion and get it completed! And please feel free to give me any suggestion.

Currently, I build some classes inheriting BaseTask to make us able to write test cases more conveniently. And I choose pytest as the unit test framework, since there are bunch of functional extensions which may help us maintain these test easily. You can check out more details in tests/README.md.

There are still some tests need to be done, I will finish them in these days.

Besides, here are some items that I would recommend to do later:

  • CI/CD (travis-ci is a good choice for GitHub project)
  • code coverage report (use pytest-cov and host the report on codecov.io)

Finally, a compatibility issue is found during the development of these tests. But I'm not sure whether it is good to send it as a new PR. In order to make it be managed easily, it's attached in this PR currently.

Copy link
Owner

@davidtvs davidtvs left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Looks good, just have a few minor comments. I might come back later with more when I run this locally.

tests/task.py Outdated Show resolved Hide resolved
tests/task.py Outdated Show resolved Hide resolved
tests/README.md Outdated Show resolved Hide resolved
@davidtvs
Copy link
Owner

davidtvs commented Apr 13, 2020

About Python 2: since in the meantime Python 2.7 has reached end of life, I think we should just remove support for it. At this point, it's just a source of pain. I'll remove everything related to it today or the coming days.

About the next steps: ya, I agree that both of those should be next. I'll probably add some github actions to check the code style too. Wanted to play around with that feature for some time now and this is a good excuse 😄

I think this is a good initial set of tests for a PR, we can add more later and keep the PRs small and low complexity.

requirements-dev.txt Outdated Show resolved Hide resolved
@davidtvs
Copy link
Owner

davidtvs commented Apr 13, 2020

Just made a PR to remove Python 2 support (#28). @NaleRaphael can you give it a look just in case I missed something?

We would then first merge #28 and then rebase this PR and remove the fix to the compatibility issue

@NaleRaphael
Copy link
Contributor Author

The fix for Py2k compatibility issue is removed. I'll submit a revision for requested changes later 😎

tests/task.py Outdated Show resolved Hide resolved
NaleRaphael added a commit to NaleRaphael/pytorch-lr-finder that referenced this pull request Apr 13, 2020
Other requested changes mentioned in PR davidtvs#27 are also done in this
commit.
Copy link
Owner

@davidtvs davidtvs left a comment

Choose a reason for hiding this comment

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

LGTM.

tests/test_lr_finder.py Outdated Show resolved Hide resolved
@davidtvs davidtvs self-requested a review April 18, 2020 19:40
@NaleRaphael
Copy link
Contributor Author

BTW, would it be better to format these code with a linter you are using? I could apply the configuration you given and submit a new commit for fixing the coding style, and it might reduce your workload on checking it. 😊
And once you think this PR is ready, please feel free to squeeze these commits to make it smaller. (Or I could do that from my side)

@davidtvs
Copy link
Owner

Just pushed my flake8 configuration. For formatting, I use black with default settings.

This is a draft of unit tests for this package. For details of how
test cases are written, please check out "tests/README.md".
Other requested changes mentioned in PR davidtvs#27 are also done in this
commit.
Local imports in `collect_task_classes()` is not necessary since
module `task` has been imported in global.
@NaleRaphael
Copy link
Contributor Author

I've updated this branch and formatted the code with black. And the following part is kept as it was in case there are more tasks to be run in the future:

# in test_lr_finder.py
@pytest.mark.parametrize(
    "cls_task",
    [
        mod_task.XORTask,
        mod_task.DiscriminativeLearningRateTask,
    ],
)
def test_reset(self, cls_task):

@davidtvs
Copy link
Owner

Looks good, thanks for the PR.

@davidtvs davidtvs merged commit 34e2380 into davidtvs:master Apr 19, 2020
@NaleRaphael NaleRaphael deleted the dev-tests branch April 20, 2020 00:46
@NaleRaphael
Copy link
Contributor Author

Thanks! More tests would be done lately in a few weeks to make the coverage rate higher, and I would open a new PR when it is ready.

davidtvs added a commit that referenced this pull request Apr 23, 2020
Thanks to @NaleRaphael that proposed the change in #27 and so I just had to copy and paste 😃
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants