Skip to content
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

Don't check for the exact zn_poly version #25311

Closed
timokau opened this issue May 8, 2018 · 15 comments
Closed

Don't check for the exact zn_poly version #25311

timokau opened this issue May 8, 2018 · 15 comments

Comments

@timokau
Copy link
Contributor

timokau commented May 8, 2018

Currently the doctests check for the version of zn_poly. That makes the doctests unnecessarily brittle. If somebody updates zn_poly they have to make a change at an unreleated position and it is an annoyance for packaging.

Component: distribution

Author: Timo Kaufmann

Branch/Commit: 83b23c8

Reviewer: Jeroen Demeyer

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

@timokau timokau added this to the sage-8.3 milestone May 8, 2018
@timokau
Copy link
Contributor Author

timokau commented May 9, 2018

Author: Timo Kaufmann

@jdemeyer
Copy link

jdemeyer commented May 9, 2018

comment:3

Why patch this specific test? I mean, I think that most of misc/package.py is problematic for packagers.

@timokau
Copy link
Contributor Author

timokau commented May 9, 2018

comment:4

Thats the only one I had to patch. I generate the files in the installed dir at build time by iterating through sages dependencies. I only have to translate two package names.

zn_poly has version 0.9 in nixpkgs, without the patch part. I could translate that just as the names, but that would need to be fixed with every change to the sage version.

And even when not considering packaging I think its best not to test for the exact version. That will always break the doctests with every zn_poly version, even if there was no api break.

@jdemeyer
Copy link

jdemeyer commented May 9, 2018

comment:5

Replying to @timokau:

Thats the only one I had to patch. I generate the files in the installed dir at build time by iterating through sages dependencies. I only have to translate two package names.

Interesting. I don't know what other distros do.

And even when not considering packaging I think its best not to test for the exact version. That will always break the doctests with every zn_poly version, even if there was no api break.

I think we specifically chose zn_poly in this doctest because it is dead upstream (last release in 2008) and the last Sage-specific patch was in 2013. So the scenario that you describe is mostly hypothetical.

@timokau
Copy link
Contributor Author

timokau commented May 9, 2018

comment:6

Replying to @jdemeyer:

Interesting. I don't know what other distros do.

At least the arch linux package just fails the package.py tests.

I think we specifically chose zn_poly in this doctest because it is dead upstream (last release in 2008) and the last Sage-specific patch was in 2013. So the scenario that you describe is mostly hypothetical.

Ah, I see. I would still prefer not to rely on this. Does that test provide any value? That installed packages are detected correctly is tested in list_packages and that the version is split from the name correctly is tested in pkgname_split.

@jdemeyer
Copy link

jdemeyer commented Jun 6, 2018

comment:7

OK, fine. Could you just replace # not tested by # random?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 6, 2018

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

e1526f8Don't test for the exact zn_poly version

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 6, 2018

Changed commit from 67a011d to e1526f8

@timokau
Copy link
Contributor Author

timokau commented Jun 6, 2018

comment:10

Done


New commits:

e1526f8Don't test for the exact zn_poly version

@jdemeyer
Copy link

jdemeyer commented Jun 6, 2018

comment:11

You are changing unrelated files

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 6, 2018

Changed commit from e1526f8 to 83b23c8

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 6, 2018

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

83b23c8Don't test for the exact zn_poly version

@timokau
Copy link
Contributor Author

timokau commented Jun 6, 2018

comment:13

Sorry, no idea how that happened. I can't remember touching those files.


New commits:

83b23c8Don't test for the exact zn_poly version

@jdemeyer
Copy link

jdemeyer commented Jun 6, 2018

Reviewer: Jeroen Demeyer

@vbraun
Copy link
Member

vbraun commented Jun 7, 2018

Changed branch from u/gh-timokau/znpoly-version to 83b23c8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants