search

tBuLi / symfit

Symbolic Fitting; fitting as it should be.
http://symfit.readthedocs.org
MIT License
233 stars 17 forks source link

Convert unittest to pytests #274

Closed antonykamp closed 4 years ago

antonykamp commented 4 years ago

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

pckroon commented 4 years ago

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

pckroon commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 :)

antonykamp commented 4 years ago

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

antonykamp commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.