Automatically test optional non-packages (CPLEX, Gurobi, Maple?, ..)

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

With this branch, the default value of --optional in sage -t is larger than the list of installed new-style optional packages.

In particular, cplex and gurobi (which are not optional packages as they are proprietary) are added to the list (some tests are too slow to be run with glpk, e.g. #18871).

More can be added to that list. If you know a better way to implement it, I am all ears.

Nathann

sage-devel thread: https://groups.google.com/d/topic/sage-devel/M98nD7833KM/discussion

Currently handles Gurobi, Cplex, Maple, Matlab, Macaulay2, Octave, Scilab.

(mathematica cannot be included before #18908 is fixed)

CC: @vbraun @jdemeyer @dcoudert

Component: doctest framework

Author: Nathann Cohen

Branch/Commit: public/18904 @ b7213ef

Reviewer: Kwankyu Lee

Issue created by migration from https://trac.sagemath.org/ticket/18904

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:45

Until somebody can explain why this 40-lines code must be moved to a (yet unnamed) new module, or what exactly should be checked if it is not 'magma(1)', ...

dimpase commented 9 years ago

comment:46

Replying to @nathanncohen:

It's not really is_X_installed, it's more like is_the_version_of_X_which_is_installed_sufficient_for_the_doctests? So, it is very much related to the doctests.

To me it would make more sense to have a version() function in sage/interfaces/mathematica.py that would return the version.

I would let .version() return -1000 if there is no program available, instead of a separate function...

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:47

I would let .version() return -1000 if there is no program available, instead of a separate function...

+1.

vbraun commented 9 years ago

comment:48

Replying to @nathanncohen:

Until somebody can explain why this 40-lines code must be moved to a (yet unnamed) new module

https://en.wikipedia.org/wiki/Single_responsibility_principle

Is the doctest controller responsible for third-party interfaces?

E.g. page 34 of http://vitoex.googlecode.com/svn/trunk/Read/Clean%20Code.pdf recommends:

"The first rule of functions is that they should be small. The second rule of functions is that they should be smaller than that. [...] Functions should not be 100 lines long. Functions should hardly ever be 20 lines long."

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:49

Is the doctest controller responsible for third-party interfaces?

Where else do you expect to see the code which decides which third-party interfaces should be tested? Are third-party interfaces responsible for doctesting? Be serious.

E.g. page 34 of http://vitoex.googlecode.com/svn/trunk/Read/Clean%20Code.pdf recommends:

"The first rule of functions is that they should be small. The second rule of functions is that they should be smaller than that. [...] Functions should not be 100 lines long. Functions should hardly ever be 20 lines long."

That a fortunate coincidence, for my function has around 26 lines of code (the rest is documentation). So I pass this test. Shoud I write myself to sage-devel to ask whether we should enforce a hard line limit of 20 lines per function?

The other good news for me is that I do not see anything about creating a new file for functions of 20< x < 30 lines.

So it's still up for review.

I think that you are officially splitting hairs.

Nathann

vbraun commented 9 years ago

comment:50

Replying to @nathanncohen:

Is the doctest controller responsible for third-party interfaces?

Where else do you expect to see the code which decides which third-party interfaces should be tested?

If you can't figure out an existing place that is responsible then your instinct should be to create a new one.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:51

If you can't figure out an existing place that is responsible then your instinct should be to create a new one.

I have no problem figuring this out. There is a place in the doctest code where we define the default list of optional packages that we should test. It is done in the DoctestController class defined in doctest.py, and I don't see a better place to write the code that builds this list.

Nathann

vbraun commented 9 years ago

comment:52

What is the DoctestController responsible for?

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:53

What is the DoctestController responsible for?

Volker, I do not know what you are trying to achieve on this ticket. What I know is that my goal here is to improve Sage's tests, so that less code gets broken.

The way we proceed is through a code review (which IIRC never involved a lot of Zen questioning): if you see something wrong with the code, you say what you think needs to be changed and why, so that we can discuss it. The 20lines limit for functions, I'm ashamed to say, it not in this ticket's scope (discuss it on sage-devel).

I now have really important things that I must see to, like tonight's tomato sauce with heaps of basil.

Nathann

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

Description changed:

--- 
+++ 
@@ -6,6 +6,9 @@

 Nathann

+sage-devel thread: https://groups.google.com/d/topic/sage-devel/M98nD7833KM/discussion
+
+
 **Currently handles** Gurobi, Cplex, Maple, Matlab, Macaulay2, Octave, Scilab.

 (mathematica cannot be included before #18908 is fixed)

vbraun commented 9 years ago

comment:55

Replying to @nathanncohen:

if you see something wrong with the code, you say what you think needs to be changed and why, so that we can discuss it.

I said it already, but let me repeat: Move the logic for detecting installed / compatible third-party packages to a separate module. This new module shall only be responsible for that one thing.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:56

I said it already, but let me repeat: Move the logic for detecting installed / compatible third-party packages to a separate module. This new module shall only be responsible for that one thing.

It would be a module containing a 40 lines function only. I do not think that it makes much sense, unless you have more code to move there.

Nathann

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:57

Moreover, please note that the code which builds the list of optional packages was already there. Before I was asked to turn this into an independent function, all that this patch did is add 10 lines of code right after other lines that already did a very similar job. So it is a bit surprising to me that all of a sudden this needs to be changed.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:58

I just had an idea: I'm done working on this patch. Whoever wants to see those packages tested can do whatever he likes, for as long as this ticket's original purpose (test Gurobi+CPLEX) works.

Have fun without me.

Nathann

dimpase commented 9 years ago

comment:59

by now, Mathematica can be added to the list. How about instead of using --optional make another parameter, say --optionalplus. Then most of the issues raised here will be gone.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

b7213ef trac #18904: Merged with 6.9.beta6

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 44dd241 to b7213ef

a1dd0ea6-9300-4f97-bb3c-0f25ba420caf commented 9 years ago

comment:61

Replying to @dimpase:

by now, Mathematica can be added to the list. How about instead of using --optional make another parameter, say --optionalplus. Then most of the issues raised here will be gone.

+1 (--external? or --thirdparty?)