meerk40t / svgelements

SVG Parsing for Elements, Paths, and other SVG Objects.
MIT License
132 stars 29 forks source link

Github: Run Unit tests automatically for every PR / commit #139

Closed Sophist-UK closed 2 years ago

tatarize commented 2 years ago

Used to have one of these, but they went away or something and Travis or whatever changed their routines.

Sophist-UK commented 2 years ago

Not sure whether you might want to implement tox (?) or similar to do automated environment setup for testing.

But if you want me to take a look when I get the time, just say...

tatarize commented 2 years ago

I used to have such a thing but then Travis which was doing it switched off some other way and stopped. I haven't bothered to do the research to find a different service but testing for other OSes and other versions of Python and not needing to do it locally might be rather nice.

Sophist-UK commented 2 years ago

I will take a look when I have time.

tatarize commented 2 years ago

Travis had some code for preinstalling that data. So you could have preinstall checks to install scipy.

https://github.com/meerk40t/svgelements/blob/master/.travis.yml

Given travis changed how they work we could probably drop that file. Unless they saw the weirdness of their way and stopped alienating people.

tatarize commented 2 years ago

Checked, nope. Still dead. If you figure out the scipy install for the current PR test-run stuff (whereas scipy is not a requirement of svgelements itself but the hypergeometric functions are for some functions), you might consider deleting the .travis.yml too since it doesn't do anything anymore.

Sophist-UK commented 2 years ago

The tests currently do separate runs for: a. No scipy or numpy b. Scipy but not numpy c. Numpy but not scipy

This is based on an assumption that all of the above are valid. It does not do Numpy and SciPy based on assumption that all code paths are tested by the above. Also an assumption (based on my interpretation of the Readme) that install of one and not the other is a valid combination.

This issue is in the test code - it is calling _length_exact when scipy is not installed - so either these tests should only be run when scipy is installed, or it should be calling length instead which handles the exception.

Sophist-UK commented 2 years ago

The GH Action tests run fine when scipy is installed, but they failed every time when it is not installed.

This is an issue in the test code, where tests that only work with scipy installed are still run when it is not installed.

tatarize commented 2 years ago

The test code requires scipy and numpy. The actual code of svgelements does not require either.

tatarize commented 2 years ago

It was pretty easy to write tests bypassing the routines that used scipy and numpy, but several of the important tests are to show that scipy code and the fallback non-scipy code produce the same results. That I can solve the length of an arc by linearization or by using the hypergeometric function. So while svgelements does not require anything, testing svgelements can and basically should require anything you need in there to test things correctly. Throw the kitchen sink at it, if you're just doing tests anything is fair game.

tatarize commented 2 years ago

Though, if you are enough of a test writing guru to disable things if the requirement isn't met, then feel free to take a stab at it. But, currently it's written assuming scipy and numpy installed fine and are perfectly fine to test against the non-enhanced code.

Sophist-UK commented 2 years ago

I am happy to limit unittest runs to include numpy and scipy - but if I do that then we will not be testing the code for any users who don't have these packages installed.

Can we make the unit tests which require these so that they only run when they are installed?

Sophist-UK commented 2 years ago

several of the important tests are to show that scipy code and the fallback non-scipy code produce the same result

The unittest jobs currently run with combinations - so we do run with numpy and we do run with scipy and so do test that they create the same results. But we currently also run without them so that we test for those users who don't have them installed.

I don't mind which way we go with this.

tatarize commented 2 years ago

There's no point in running the tests multiple times with the different super-optional dependencies. These need those to test the functions but they can't do different things without them. It's not worth running the same tests multiple times with code that can't operate differently.

Sophist-UK commented 2 years ago

Ok - I will change the unittest code to install scipy and numpy regardless.

But if we ever get a situation where user without either scipy or numpy has a problem, expect me to say "Told you so" and blow a raspberry.

tatarize commented 2 years ago

The reason for the tests is so that we can know that they do not need these. In fact the numpy stuff has a different method of calling the code. Namely it calls npoint() which is like the point() function but np like numpy and like n the metastatic variable simultaneous points. abey79 is pretty awesome with that stuff. The scipy thing has pretty solid fallbacks. Though admittedly inpain has managed to have a faulty version of scipy crash for other reasons than failure to import and the exception there got broader and broader since the point is try this and if it fails for any reason do the slower harder way of doing this.

Sophist-UK commented 2 years ago

I think the broad exception is correct - however if there is any other issue other than the import not existing then the user should be informed (once rather than every time it fails to import) that there is a problem. Using an exception to allow things to keep going when there is a genuine underlying issue is OK - but we have to report the problem to the user so that they know about it.