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

Convert unittest to pytests #274

Merged
merged 23 commits into from Dec 10, 2019
Merged

Convert unittest to pytests #274

merged 23 commits into from Dec 10, 2019

Conversation

antonykamp
Copy link
Contributor

This PR includes the first stage of test-conversion.
At the moment it's a pure translation of unittest to pytest

@pckroon
Copy link
Collaborator

pckroon commented Dec 5, 2019

I'll make some time next week to review and merge this. Sorry for the long wait.

@pckroon
Copy link
Collaborator

pckroon commented Dec 6, 2019

So I made a few mistakes/tpoes in merging in master. Could you rebase on master squashing 82bc99f and cf1bc92 into be69afc? I'd do it for you, but I don't know what will then happen with the commit authorships.
In the meantime I found a few fishy linebreaks, which I'll fix.

@antonykamp
Copy link
Contributor Author

I'm not quite sure if this is the newest version of the conversion because the whole discussion took place in the other PR.

@pckroon
Copy link
Collaborator

pckroon commented Dec 6, 2019

I wasn't sure which one was newer. But IMO #268 grew way too large and unwieldy. This one looked like the one where you rebased your previous work, and grouped the commits per file, as I requested in #268.

@pckroon
Copy link
Collaborator

pckroon commented Dec 6, 2019

I think that were all the blemishes. Could you 1) rebase it such that my merge typoes get squashed? If you want to, feel free to also treat my cleanup commits as squashes or fixups. 2) go over it one last time to make sure I didn't miss anything?
If it all looks good I'll merge this early next week. This looks like a very good starting point to continue improving the tests. Thanks for doing all the heavy lifting :)

Remove unused imports

Remove unused imports

Removed unneeded line endings

Remove header in test_finite_difference

Remove unneeded empty lines
@antonykamp
Copy link
Contributor Author

I squashed the typos and cleanups together. In my opinion, it's the cleanest possibility ;)
I'll take a look over it tm.

@antonykamp
Copy link
Contributor Author

I took a look over the tests and I have some questions/ideas :D

First of all: A testcase was commented out in test_general. I would convert the function into pytest-code and mark it as skipped.

In addition, many lines of code were commented out, because they are irrelevant. For example in test_general and test_auto_fit. Either I remove such irrelevant lines in this PR or in another PR (or never...maybe they are relevant).

In addition, I have found a rather long line in test_argument, but I can not think of an elegant linebreak.

@pckroon
Copy link
Collaborator

pckroon commented Dec 9, 2019

First of all: A testcase was commented out in test_general. I would convert the function into pytest-code and mark it as skipped.

Sure. It would be good to later have a look at the comment ("redudant with test_error_analytical?") and reevaluate whether we need the test. Otherwise we should just remove it completely.

In addition, many lines of code were commented out, because they are irrelevant. For example in test_general and test_auto_fit. Either I remove such irrelevant lines in this PR or in another PR (or never...maybe they are relevant).

Leave them for now. Once we start really redoing the tests pytest-style they should probably be just removed.

In addition, I have found a rather long line in test_argument, but I can not think of an elegant linebreak.

Leave it for now. @tBuLi doesn't really have a problem with long lines (much less than me). At some point I need to talk to him about pep8 and pylint though...

This PR looks good to me as is. I'll give @tBuLi until noon tomorrow to intervene before I merge it :)

@pckroon pckroon merged commit ecc567a into tBuLi:master Dec 10, 2019
@pckroon
Copy link
Collaborator

pckroon commented Dec 10, 2019

Thanks for all the work, I merged it :)

Going forward with the whole pytest parametrize idea: pick a test file, and convert/adapt that. Make a PR, get that merged, then continue to the next. So long as the coverage doesn't drop/change you get a lot of liberties in rewriting/removing/adding tests, as far as I'm concerned. Also feel free to separate specific test cases (usually related to now-closed issues) to a separate file. I think test_finite_difference.py would be a good starting point since it's small and fairly structured already.
I should have a bit of time available the coming weeks since I managed to finish most of my thesis. Feel free to @-mention me if we're (too) slow in responding, as happened on this PR.

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.

None yet

2 participants