#30410 closed enhancement (fixed)

Command "sage -tox"

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.2
Component: doctest framework Keywords:
Cc: gh-tobiasdiez, dimpase, jhpalmieri, chapoton. Merged in:
Authors: Matthias Koeppe Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: eba708b (Commits, GitHub, GitLab) Commit: eba708b89b9935f86d6db8369ef83bcfa1ca9d4b
Dependencies: #30408 Stopgaps:

Status badges

Description (last modified by mkoeppe)

This will run tox using src/tox.ini.

Entry point for doctesting and linting: See ./sage -tox -l -v or (cd src && tox -l -v).

Change History (26)

comment:1 Changed 16 months ago by mkoeppe

  • Branch set to u/mkoeppe/command__sage__tox_

comment:2 Changed 16 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Commit set to 5ccccbb0f7a28c878646fcf5c1580cba0e26c475
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

5ccccbbsage --tox

comment:3 follow-up: Changed 16 months ago by gh-tobiasdiez

Instead of putting more into the bin/sage script, wouldn't it make sense to actually extract all building and testing related code to tox? So instead of sage -b (sage -t) you run tox build (tox test respectively). This would leave the sage script only with the functionality that a user needs to run sage.

(also I don't see any advantages of sage -tox pycodestyle over tox pycodestyle)

Last edited 16 months ago by gh-tobiasdiez (previous) (diff)

comment:4 Changed 16 months ago by gh-tobiasdiez

  • Dependencies set to 30361

Whatever the final interface looks like, the documentation needs to be changed as well to reflect this. In particular, this ticket depends on #30361.

comment:5 in reply to: ↑ 3 Changed 16 months ago by mkoeppe

Replying to gh-tobiasdiez:

Instead of putting more into the bin/sage script, wouldn't it make sense to actually extract all building and testing related code to tox? So instead of sage -b (sage -t) you run tox build (tox test respectively). This would leave the sage script only with the functionality that a user needs to run sage.

Some projects are indeed using tox also for building, but I think I would like to keep it for testing only. At least for now - we can't make too many changes at the same time so that our developer community is not overwhelmed.

sage -coverage and sage -startuptime, however, should definitely become tox environments.

(also I don't see any advantages of sage -tox pycodestyle over tox pycodestyle)

The difference is that sage -tox would also work with src/tox.ini when called from SAGE_ROOT. (There's another tox.ini in SAGE_ROOT for a different purpose - testing of the sage distribution.)

And it can give a hint to install tox when it is not available.

comment:6 Changed 16 months ago by mkoeppe

  • Dependencies changed from 30361 to #30408, #30361

comment:7 Changed 16 months ago by git

  • Commit changed from 5ccccbb0f7a28c878646fcf5c1580cba0e26c475 to 857444889f93a3053dda3e195444b3560b35ad62

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

1cedc00Add configuration of pycodestyle via tox
dde3b4fsrc/tox.ini: Add testenv:pycodestyle
939e9e0src/tox.ini: Better default arg testenv:pycodestyle
583bde5Merge branch 't/30408/public/build/pycodestyleConfig' into t/30410/command__sage__tox_
8574448src/tox.ini: Add coverage, startuptime; refactor, add descriptions

comment:8 Changed 16 months ago by mkoeppe

  • Dependencies changed from #30408, #30361 to #30408
  • Description modified (diff)

I don't think #30361 needs to be a dependency.

comment:9 follow-up: Changed 16 months ago by gh-tobiasdiez

It should also be possible to add simple forwards in the tox.ini at the root folder of the form

[testenv:pycodestyle]
whitelist_externals=tox
commands=tox -c {toxinidir}/src/tox.ini -e pycodestyle

Then you can simply call tox -e pycodestyle.

comment:10 follow-up: Changed 16 months ago by gh-tobiasdiez

I also think the testing methods in bin/sage should now be removed or deprecated (or at least use tox), or not?

comment:11 in reply to: ↑ 9 Changed 16 months ago by mkoeppe

Replying to gh-tobiasdiez:

It should also be possible to add simple forwards in the tox.ini at the root folder

Good idea, will do.

comment:12 in reply to: ↑ 10 ; follow-up: Changed 16 months ago by mkoeppe

Replying to gh-tobiasdiez:

I also think the testing methods in bin/sage should now be removed or deprecated (or at least use tox), or not?

For that we would first have to make tox a "standard" package. Step by step...

comment:13 in reply to: ↑ 12 Changed 16 months ago by mkoeppe

Replying to mkoeppe:

Replying to gh-tobiasdiez:

I also think the testing methods in bin/sage should now be removed or deprecated (or at least use tox), or not?

For that we would first have to make tox a "standard" package. Step by step...

Opened #30416 for that.

comment:14 Changed 16 months ago by git

  • Commit changed from 857444889f93a3053dda3e195444b3560b35ad62 to eba708b89b9935f86d6db8369ef83bcfa1ca9d4b

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

eba708btox.ini: Delegate doctest, coverage, startuptime, pycodestyle to src/tox.ini

comment:15 Changed 16 months ago by mkoeppe

Ready for review

comment:16 follow-up: Changed 16 months ago by gh-tobiasdiez

Looks good to me.

The only point where I'm not sure is whether the src/tox.ini file is really needed, or if it can be combined with the tox.ini in the root directory. Similarly, I'm not sure if sage -tox should always be relative to the tox config in the src folder.

comment:17 in reply to: ↑ 16 Changed 16 months ago by mkoeppe

Replying to gh-tobiasdiez:

The only point where I'm not sure is whether the src/tox.ini file is really needed, or if it can be combined with the tox.ini in the root directory.

Yes, I have been debating this with myself actually, but in the end I think it is important to keep src/tox.ini simple and readable -- the portability testing stuff in the root can be overwhelming to developers.

Similarly, I'm not sure if sage -tox should always be relative to the tox config in the src folder.

The purpose of this is to make it easy to run the tests on user code -- in the same way that sage -t is often used.

comment:18 follow-up: Changed 16 months ago by dimpase

I've tried it on a file (with system-wide tox and python):

sage-dev/src $ ../sage -tox  sage/graphs/generators/distance_regular.pyx 
doctest run-test-pre: PYTHONHASHSEED='2313650994'
doctest run-test: commands[0] | sh -c '/mnt/opt/Sage/sage-dev/src/../sage -t -p 0 sage/graphs/generators/distance_regular.pyx'
Running doctests with ID 2020-08-27-17-20-03-d333763b.
Git branch: HEAD
Using --optional=build,dochtml,e_antic,gap_packages,libsemigroups,meataxe,memlimit,normaliz,pynormaliz,sage
Doctesting 1 file using 4 threads.
sage -t --warn-long 108.5 --random-seed=0 sage/graphs/generators/distance_regular.pyx
    [123 tests, 172.32 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 172.4 seconds
    cpu time: 172.5 seconds
    cumulative wall time: 172.3 seconds
coverage run-test-pre: PYTHONHASHSEED='2313650994'
coverage run-test: commands[0] | sh -c 'if [ -z "sage/graphs/generators/distance_regular.pyx" ]; then /mnt/opt/Sage/sage-dev/src/../sage --coverageall; else /mnt/opt/Sage/sage-dev/src/../sage --coverage sage/graphs/generators/distance_regular.pyx; fi'
------------------------------------------------------------------------
SCORE sage/graphs/generators/distance_regular.pyx: 100.0% (30 of 30)
------------------------------------------------------------------------
startuptime run-test-pre: PYTHONHASHSEED='2313650994'
startuptime run-test: commands[0] | sh -c '/mnt/opt/Sage/sage-dev/src/../sage --startuptime sage/graphs/generators/distance_regular.pyx'
[]
Traceback (most recent call last):
  File "/mnt/opt/Sage/sage-dev/src/bin/sage-startuptime.py", line 131, in <module>
    raise ValueError('"' + module_arg + '" does not uniquely determine Sage module.')
ValueError: "sage/graphs/generators/distance_regular.pyx" does not uniquely determine Sage module.
ERROR: InvocationError for command /bin/sh -c '/mnt/opt/Sage/sage-dev/src/../sage --startuptime sage/graphs/generators/distance_regular.pyx' (exited with code 1)
pycodestyle installed: pycodestyle==2.6.0
pycodestyle run-test-pre: PYTHONHASHSEED='2313650994'
pycodestyle run-test: commands[0] | pycodestyle sage/graphs/generators/distance_regular.pyx
  unknown option 'description' ignored
sage/graphs/generators/distance_regular.pyx:547:28: E701 multiple statements on one line (colon)
ERROR: InvocationError for command /mnt/opt/Sage/sage-dev/src/.tox/pycodestyle/bin/pycodestyle sage/graphs/generators/distance_regular.pyx (exited with code 1)
___________________________________________________________________________ summary ___________________________________________________________________________
  doctest: commands succeeded
  coverage: commands succeeded
ERROR:   startuptime: commands failed
ERROR:   pycodestyle: commands failed

not sure I understand the errors from pycodestyle and startuptime - do I have everything set up right?

Is it possible to do only, say, pycodestyle part?


pycodestyle output kind of makes sense (except that unknown option 'description' ignored thing)

does startuptime only work for modules? (e.g. sage/graphs/) ?

Last edited 16 months ago by dimpase (previous) (diff)

comment:19 in reply to: ↑ 18 Changed 16 months ago by mkoeppe

Replying to dimpase:

Is it possible to do only, say, pycodestyle part?

Yes, tox -e pycodestyle

comment:20 Changed 16 months ago by mkoeppe

startuptime unfortunately has an inconsistent interface that wants a module name rather than a filename. Something that should probably be fixed

comment:21 Changed 16 months ago by dimpase

can this be mentioned in the dev manual? then it would be a positive review.

comment:22 Changed 16 months ago by mkoeppe

  • Dependencies changed from #30408 to #30408, #30361

OK, will do, adding to the documentation section from #30361

comment:23 Changed 16 months ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

comment:24 Changed 16 months ago by mkoeppe

  • Dependencies changed from #30408, #30361 to #30408

Thanks!

comment:25 Changed 16 months ago by mkoeppe

Follow-up at #30452 and #30453

comment:26 Changed 15 months ago by vbraun

  • Branch changed from u/mkoeppe/command__sage__tox_ to eba708b89b9935f86d6db8369ef83bcfa1ca9d4b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.