-
-
Notifications
You must be signed in to change notification settings - Fork 452
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
Test all installed optional packages by default #18558
Comments
This comment has been minimized.
This comment has been minimized.
Branch: u/ncohen/18558 |
Commit: |
comment:2
See also #13540. |
comment:3
Cool, a 3 years old ticket without a branch. I will drop them a line. Nathann |
comment:4
Replying to @nathanncohen:
Well, there is a patch. |
comment:5
Note that tests for the |
comment:6
It is two years old only. Which do you prefer to review, theirs or mine?
True, I will create another ticket in a second (done: #18559) Nathann |
comment:7
Instead of patching |
comment:8
And instead of naming the option |
comment:9
I did not patch the doctest framework because the default value of 'optional' is systematically overwritten by |
comment:10
Replying to @nathanncohen:
What do you mean with this? The |
comment:11
Replying to @nathanncohen:
I meant "add a doctest in the doctest framework", but I admit I haven't thought much about what this should be. |
comment:12
On the topic of the option name: make it just |
comment:13
I mean that I was surprised to find that the |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:15
There is now an Nathann |
comment:17
Is there going to be a separate |
comment:127
Replying to @jdemeyer:
I am fixing most mistakes in the above list in #18621 and #18637. |
comment:128
An interesting thing that comes up with #18637's fixes is that I don't think the However, I recognize that may be orthogonal enough (or difficult enough) to not bother with here, and instead better to deal with elsewhere. |
comment:129
Replying to @kcrisman:
They are not run automatically run. This ticket is only about optional Sage packages.
Yes, see #13540 |
comment:130
I think it's easy to automatically run the internet tests. I don't actually know if that's a good idea: how often will someone start doctesting with internet access (at which point the tag will get added to the list) and then lose it during doctesting (they lose their internet connection, they take their laptop outside, etc.)? Adapted from #13540: diff --git a/src/sage/doctest/control.py b/src/sage/doctest/control.py
index ce846b2..7b07f2d 100644
--- a/src/sage/doctest/control.py
+++ b/src/sage/doctest/control.py
@@ -256,6 +256,14 @@ class DocTestController(SageObject):
for pkg, versions in optional_pkgs.items():
if versions[0] == versions[1]:
options.optional.add(pkg)
+ # If there is an internet connection, add
+ # 'internet' to the list of optional tags.
+ try:
+ import urllib2
+ urllib2.urlopen('http://sagemath.org/')
+ options.optional.add('internet')
+ except urllib2.URLError:
+ pass
# Check that all tags are valid
for o in options.optional: I also expect lots of failing internet doctests, and I don't know how to fix all of them. For example, I don't understand the doctesting structure for the files in Of course this all belongs on another ticket, either #13540 or a piece of it. |
comment:131
Actually, for reasons I really don't understand, my previous code with
without the line Anyway, the following doesn't produce segfaults (taken from stackoverflow): diff --git a/src/sage/doctest/control.py b/src/sage/doctest/control.py
index ce846b2..ff365dd 100644
--- a/src/sage/doctest/control.py
+++ b/src/sage/doctest/control.py
@@ -256,6 +256,15 @@ class DocTestController(SageObject):
for pkg, versions in optional_pkgs.items():
if versions[0] == versions[1]:
options.optional.add(pkg)
+ # If there is an internet connection, add
+ # 'internet' to the list of optional tags.
+ try:
+ import socket
+ host = socket.gethostbyname('sagemath.org')
+ socket.create_connection((host, 80), 2)
+ options.optional.add('internet')
+ except:
+ pass
# Check that all tags are valid
for o in options.optional: |
comment:132
Finally, for the actual ticket under discussion, I am happy with it. Anyone have any remaining objections? |
comment:133
My brain is not quite working today. I am almost happy with the current ticket. I am not sure about the message |
Changed reviewer from Jeroen Demeyer, Karl-Dieter Crisman to Jeroen Demeyer, Karl-Dieter Crisman, John Palmieri |
comment:134
That would suffice to address my very similar concern in comment:117 as well. |
comment:135
How about
|
comment:136
Optional packages that are outside of sage (like graphviz and mathematica) are currently not detected as tags. With mathematica 10.0 and sage 6.8.beta3 and graphviz and this ticket I get:
but
Similarly, less tests are done because the tag graphviz is not detected:
compared to
|
comment:137
Replying to @seblabbe:
I know, but that's outside the scope of this ticket. |
This comment has been minimized.
This comment has been minimized.
comment:138
Okay. I just installed as many optional packages that I could. Below is the list of my optional tags:
I am starting a |
comment:139
Replying to @jdemeyer:
Sounds okay to me. |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:142
Replying to @seblabbe:
|
Changed branch from u/jdemeyer/18558 to |
With this branch, running "sage -t" will automatically involve testing all installed up-to-date new-style optional Sage packages.
Depends on #18456
Depends on #18124
Depends on #18559
Depends on #18563
Depends on #18579
Depends on #18581
CC: @vbraun @jdemeyer @sagetrac-tmonteil @seblabbe
Component: doctest framework
Author: Nathann Cohen, Jeroen Demeyer
Branch/Commit:
82f3eec
Reviewer: Jeroen Demeyer, Karl-Dieter Crisman, John Palmieri
Issue created by migration from https://trac.sagemath.org/ticket/18558
The text was updated successfully, but these errors were encountered: