Opened 7 years ago
Closed 7 years ago
#14344 closed defect (fixed)
Fix packages test
Reported by: | kcrisman | Owned by: | mvngu |
---|---|---|---|
Priority: | blocker | Milestone: | sage-5.9 |
Component: | doctest coverage | Keywords: | |
Cc: | kini, jhpalmieri | Merged in: | sage-5.9.beta2 |
Authors: | Keshav Kini | Reviewers: | Karl-Dieter Crisman |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
Here is a request from a sage-devel thread:
The following test fails because I have some optional packages installed - $ sage -t --long -force_lib devel/sage/sage/misc/package.py sage -t --long -force_lib "devel/sage/sage/misc/package.py" ********************************************************************** File "/home/rajeev/software/general/sage/devel/sage/sage/misc/package.py", line 119: sage: install_package() Expected: ['atlas...', 'blas...', ...] Got: ['PyQt_x11-4.9.1.p0', 'atlas-3.8.4.p1', 'blas-20070724', 'boehm_gc-7.2.alpha6.p2', 'boost-cropped-1.34.1',
The consensus seemed to be simply adding more ellipses.
> Just add periods *before* 'atlas...' as well. And after it (between 'atlas...' and 'blas...'), just in case. Maybe something like ['atlas...', ..., 'python...', ...] This is a better idea, IMO.
Attachments (1)
Change History (10)
comment:1 Changed 7 years ago by
comment:2 Changed 7 years ago by
This patch isn't compatible with #10508, and goodness knows that ticket doesn't need any more dependencies... is it okay to ask to rebase to a "needs work" ticket? Maybe the version with python
as John suggested? Or the filter version you mentioned in the relevant thread, if you think that one is better (though perhaps it's harder to understand what it's doing).
comment:3 Changed 7 years ago by
How's this patch?
The filter thing I posted on sage-devel is semantically nicer, IMO, but I agree it looks pretty ugly and hard to read in Python, and this test isn't exactly run in a tight loop so performance is sort of irrelevant :)
comment:4 Changed 7 years ago by
I just realized that probably they would still need to rebase at #10508, so I may have wasted your time... anyway, looks good. I had some trouble finding an spkg that would actually compile on my box that was before atlas, though, to make sure the test worked! The one in the thread needs qmake, and the ace package is pretty outdated, apparently, and seems to remove some stuff it maybe shouldn't. Anyway, 4ti2 worked and the test still passes, so we're fine.
comment:5 Changed 7 years ago by
- Reviewers set to Karl-Dieter Crisman
- Status changed from new to needs_review
comment:6 Changed 7 years ago by
- Status changed from needs_review to positive_review
comment:7 Changed 7 years ago by
This is such a tiny patch, I really couldn't complain about any time being wasted :) Since you've given this positive review, I'll go rebase the patch at #10508 on this.
comment:8 Changed 7 years ago by
- Priority changed from minor to blocker
comment:9 Changed 7 years ago by
- Merged in set to sage-5.9.beta2
- Resolution set to fixed
- Status changed from positive_review to closed
Here's a patch.