sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.45k stars 481 forks source link

make *test*: Log to a common log file test.log #33995

Closed mkoeppe closed 2 years ago

mkoeppe commented 2 years ago

... instead of separate logs ptest.log, ptestalllong.log, etc.

Before the test run, the actual command line (sage -t) is now shown in the log.

We also deprecate make testoptional etc., which are identical to the targets without optional.

We also modify the targets test-nodoc etc. so that they skip all tests that depend on the documentation.

(split out from #33262)

Depends on #33823 Depends on #33967

CC: @jhpalmieri

Component: doctest framework

Author: Matthias Koeppe

Branch/Commit: fd44af2

Reviewer: John Palmieri

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

mkoeppe commented 2 years ago

Dependencies: #33823, #33967

mkoeppe commented 2 years ago

Branch: u/mkoeppe/common_log_file_test_log

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Commit: a1785b6

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

a1785b6Makefile (*test*): Use recursive make invocations for delegation, deprecate *testoptional* targets
mkoeppe commented 2 years ago

Description changed:

--- 
+++ 
@@ -2,6 +2,9 @@

 Before the test run, the actual command line (`sage -t`) is now shown in the log.

+We also deprecate `make testoptional` etc., which are identical to the targets without `optional`.
+
+We also modify the targets `test-nodoc` etc. so that they skip all tests that depend on the documentation.

 (split out from #33262)
-
+ 
jhpalmieri commented 2 years ago
comment:5

This isn't working for me: running make ptest doesn't run things in parallel, and running make testlong doesn't add --long to the flags. Here is the top of test.log after deleting the old test.log and running make testlong:

### Running ./sage -t --logfile=logs/test.log  --optional=sage,optional --all
Running doctests with ID 2022-06-22-21-50-38-9f6b3799.
Git branch: t/33995/common_log_file_test_log
Git ref: 9.7.beta3-10-g49b60ed906
Running with SAGE_LOCAL='/Users/palmieri/Desktop/Sage/sage_builds/TESTING/sage-9.7.beta3/local' and SAGE_VENV='/Users/palmieri/Desktop/Sage/sage_builds/TESTING/sage-9.7.beta3/local/var/lib/sage/venv-python3.9'
Using --optional=homebrew,pip,sage,sage_spkg
Features to be detected: 4ti2,benzene,bliss,buckygen,conway_polynomials,csdp,database_cremona_ellcurve,database_cremona_mini_ellcurve,database_jones_numfield,database_knotinfo,dvipng,gfan,graphviz,imagemagick,jupymake,kenzo,latte_int,lrslib,mcqd,meataxe,msolve,nauty,palp,pandoc,pdf2svg,pdftocairo,phitigra,plantri,polytopes_db,polytopes_db_4d,pynormaliz,python_igraph,rubiks,sage.combinat,sage.geometry.polyhedron,sage.graphs,sage.groups,sage.plot,sage.rings.number_field,sage.rings.padics,sage.rings.real_double,sage.symbolic,sage_numerical_backends_coin,sagemath_doc_html,sphinx,tdlib
Doctesting entire Sage library.
Doctesting all documentation sources.
Doctesting 4379 files.
sage -t --random-seed=329698041836605561446928060805522518137 src/sage/all_cmdline.py

By the way, I like the message Running ./sage -t .... Should we also include the make target? I'm not sure how to phrase it, but something like Invoked by 'make $@': running ./sage -t ....

jhpalmieri commented 2 years ago
comment:6

Parts work: running make ptestlong-nodoc skips the right files. The start of test.log:

### Running ./sage -t --logfile=logs/test.log  --optional=sage,optional,!sagemath_doc_html,!sagemath_doc_pdf --all
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

ea9001aMakefile (*logs*): Use variable SAGE_ROOT_LOGS
8e32ba6Makefile (*test*-nodoc): Skip tests marked # optional - sagemath_doc_html or sagemath_doc_pdf
1248371Makefile (*test*): Log to a common log file TEST.log
159bae1Makefile: testoptional is the same as test
d5b5f8eMakefile: Rewrite *test* targets using target-specific variables TEST_FLAGS, TEST_OPTIONAL
fc7135cMakefile (test): Back to using test.log (not TEST.log) for now
f690812Makefile (*test*): Use recursive make invocations for delegation, deprecate *testoptional* targets
52a12e5Makefile (*test*): Pass TEST_FLAGS, TEST_OPTIONAL as arguments
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from a1785b6 to 52a12e5

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from 52a12e5 to fd44af2

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

fd44af2Makefile (*test*): Print the makefile target in the log file
mkoeppe commented 2 years ago
comment:9

Replying to @jhpalmieri:

This isn't working for me: running make ptest doesn't run things in parallel [...]

Indeed, fixed now.

mkoeppe commented 2 years ago
comment:10

Replying to @jhpalmieri:

I like the message Running ./sage -t .... Should we also include the make target? I'm not sure how to phrase it, but something like Invoked by 'make $@': running ./sage -t ....

Done now

jhpalmieri commented 2 years ago
comment:11

Looks great now. What do you think of changes like the following?

diff --git a/Makefile b/Makefile
index dd9c1567add..3cdb11d7f9e 100644
--- a/Makefile
+++ b/Makefile
@@ -274,7 +274,7 @@ ptestalllong:
        @$(MAKE) TEST_TARGET="$(TEST_TARGET)" TEST_FLAGS="$(TEST_FLAGS) --long" ptestall

 testoptional:
-       @echo "'make $@' is deprecated; use 'make test'"
+       @echo "'make $@' is deprecated; use 'make test'" >> $(TEST_LOG)
        @$(MAKE) test

 testoptionallong:

I don't know the best way to make these notifications visible.

mkoeppe commented 2 years ago
comment:12

I think this message belongs on the console, not the log

jhpalmieri commented 2 years ago
comment:13

How about both places?

diff --git a/Makefile b/Makefile
index dd9c1567add..49d74bd6eaa 100644
--- a/Makefile
+++ b/Makefile
@@ -274,7 +274,7 @@ ptestalllong:
        @$(MAKE) TEST_TARGET="$(TEST_TARGET)" TEST_FLAGS="$(TEST_FLAGS) --long" ptestall

 testoptional:
-       @echo "'make $@' is deprecated; use 'make test'"
+       @echo "'make $@' is deprecated; use 'make test'" | tee -a $(TEST_LOG)
        @$(MAKE) test

 testoptionallong:

Messages in either case are likely to get buried. When I run make testoptional, this deprecation is the first thing that appears, but so much other stuff gets printed right away, that it scrolls off of the screen, so I think that people won't see it if they're not actively looking for it. The same may be true for the log files. We could try highlighting the warning by putting some blank lines before and after, or a line of ****** or whatever. Is the warning worth highlighting?

mkoeppe commented 2 years ago
comment:14

Fine with me if you want to push this change using tee to the ticket. I agree that the deprecation notice is easily overlooked, but I don't think it's very important

jhpalmieri commented 2 years ago

Reviewer: John Palmieri

jhpalmieri commented 2 years ago
comment:15

Let's leave it as is.

mkoeppe commented 2 years ago
comment:16

Thanks!

vbraun commented 2 years ago

Changed branch from u/mkoeppe/common_log_file_test_log to fd44af2