Opened 3 years ago

Closed 3 years ago

#25311 closed enhancement (fixed)

Don't check for the exact zn_poly version

Reported by: gh-timokau Owned by:
Priority: major Milestone: sage-8.3
Component: distribution Keywords:
Cc: Merged in:
Authors: Timo Kaufmann Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 83b23c8 (Commits, GitHub, GitLab) Commit: 83b23c8eab60819c9697366d77004181db5b2745
Dependencies: Stopgaps:

Status badges

Description

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.

Change History (15)

comment:1 Changed 3 years ago by gh-timokau

  • Type changed from PLEASE CHANGE to enhancement

comment:2 Changed 3 years ago by gh-timokau

  • Authors set to Timo Kaufmann
  • Status changed from new to needs_review

comment:3 Changed 3 years ago by jdemeyer

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

comment:4 follow-up: Changed 3 years ago by gh-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.

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.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 3 years ago by jdemeyer

Replying to gh-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.

comment:6 in reply to: ↑ 5 Changed 3 years ago by gh-timokau

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.

comment:7 Changed 3 years ago by jdemeyer

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

comment:8 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:9 Changed 3 years ago by git

  • Commit changed from 67a011dc8580f945b4f62c986662130b880a2836 to e1526f8a9349b1500d14705cc8c68cf25a88a060

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

comment:10 Changed 3 years ago by gh-timokau

  • Status changed from needs_work to needs_review

Done


New commits:

e1526f8Don't test for the exact zn_poly version

comment:11 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

You are changing unrelated files

comment:12 Changed 3 years ago by git

  • Commit changed from e1526f8a9349b1500d14705cc8c68cf25a88a060 to 83b23c8eab60819c9697366d77004181db5b2745

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

comment:13 Changed 3 years ago by gh-timokau

  • Status changed from needs_work to needs_review

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


New commits:

83b23c8Don't test for the exact zn_poly version

comment:14 Changed 3 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:15 Changed 3 years ago by vbraun

  • Branch changed from u/gh-timokau/znpoly-version to 83b23c8eab60819c9697366d77004181db5b2745
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.