GNU bug report logs - #74394
[PATCH 0/2] Skip slow tests by default and run 'check' in Git pre-push hook.

Previous Next

To reply to this bug, email your comments to 74394 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: guix-patches <at> gnu.org
Cc: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Subject: [PATCH 0/2] Skip slow tests by default and run 'check' in Git
 pre-push hook.
Date: Sun, 17 Nov 2024 21:03:30 +0900

Hello,

This is a simple change that should ensure test suite breakages are detected
as early as possible and avoid tests breaking changes to be pushed.  This is
made possible by skipping a few expensive tests suite, bringing down the total
test time to about 1 minute on a fast machine.

We could call it a "distributed CI" approach ;-).

Note: I initially pursued an Automake or Make-based approach, but it ended up
far from trivial, hitting old issues such as [0] along the way.  This solution
simply puts the skip logic in the tests that must be skipped (a one liner).

To run the complete test suite including the slow tests (as is the case prior
this change):

make check WITH_SLOW_TESTS=1

[0]  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=74387

Maxim Cournoyer (2):
  build: Exclude expensive tests in check target by default.
  etc: Ensure test suite passes in pre-push git hook.

 Makefile.am                | 9 ++++++++-
 etc/git/pre-push           | 1 +
 tests/guix-home.sh         | 5 +++++
 tests/guix-package.sh      | 5 +++++
 tests/guix-system.sh       | 4 ++++
 tests/guix-time-machine.sh | 4 +++-
 6 files changed, 26 insertions(+), 2 deletions(-)


base-commit: 94133452aa49de672d69950b2e1a99432111074c
-- 
2.46.0

Message #8 received at 74394 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: 74394 <at> debbugs.gnu.org
Cc: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Subject: [PATCH 1/2] build: Exclude expensive tests in check target by default.
Date: Sun, 17 Nov 2024 21:21:46 +0900

This reduces the test time of 'make -j check' from ~22 minutes to a bit more than one
minute on a fast machine.

* Makefile.am [CAN_RUN_TESTS] (check-local): New target.
* tests/guix-home.sh: Skip unless WITH_SLOW_TESTS is 1.
* tests/guix-package.sh: Likewise.
* tests/guix-system.sh: Likewise.
* tests/guix-time-machine.sh: Exclude one command.

Change-Id: Ieb61bf5d54aedd4bd32f93da792a4ac7dd8b9e7d
---
 Makefile.am                | 9 ++++++++-
 tests/guix-home.sh         | 5 +++++
 tests/guix-package.sh      | 5 +++++
 tests/guix-system.sh       | 4 ++++
 tests/guix-time-machine.sh | 4 +++-
 5 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index e94ba87797..4bec4d4a06 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -14,7 +14,7 @@
 # Copyright © 2018 Oleg Pykhalov <go.wigust <at> gmail.com>
 # Copyright © 2018 Alex Vong <alexvong1995 <at> gmail.com>
 # Copyright © 2019, 2023 Efraim Flashner <efraim <at> flashner.co.il>
-# Copyright © 2020, 2021, 2023 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
+# Copyright © 2020, 2021, 2023, 2024 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
 # Copyright © 2021 Chris Marusich <cmmarusich <at> gmail.com>
 # Copyright © 2021 Andrew Tropin <andrew <at> trop.in>
 # Copyright © 2023 Clément Lassieur <clement <at> lassieur.org>
@@ -668,6 +668,13 @@ tests/guix-gc.log:							\
   $(patsubst %.sh,%.log,$(filter-out tests/guix-gc.sh,$(SH_TESTS)))	\
   $(SCM_TESTS:%.scm=%.log)
 
+check-local:
+	@echo
+	@if [ "$$WITH_SLOW_TESTS" != 1 ]; then \
+	   echo "Skipping slow tests; set WITH_SLOW_TESTS to 1 to run them"; \
+	fi
+	@echo
+
 else !CAN_RUN_TESTS
 
 TESTS =
diff --git a/tests/guix-home.sh b/tests/guix-home.sh
index 649d811a0c..1c7b6da1cd 100644
--- a/tests/guix-home.sh
+++ b/tests/guix-home.sh
@@ -22,6 +22,11 @@
 # Test the 'guix home' using the external store, if any.
 #
 
+# This test is very expensive as guix-home appears to refer to the
+# current-guix package, which must be built the first time it runs (~20
+# minutes on a fast machine).
+test "$WITH_SLOW_TESTS" != 1 && exit 77
+
 set -e
 
 guix home --version
diff --git a/tests/guix-package.sh b/tests/guix-package.sh
index 945d59cdfb..5b9af19153 100644
--- a/tests/guix-package.sh
+++ b/tests/guix-package.sh
@@ -22,6 +22,11 @@
 # Test the `guix package' command-line utility.
 #
 
+# This test contains expensive operations such as 'guix package -A', which
+# cumulate into a total test time of multiple minutes (about 2 minutes on a
+# fast machine).
+test "$WITH_SLOW_TESTS" != 1 && exit 77
+
 guix package --version
 
 readlink_base ()
diff --git a/tests/guix-system.sh b/tests/guix-system.sh
index 99147cf332..bd343ec4bc 100644
--- a/tests/guix-system.sh
+++ b/tests/guix-system.sh
@@ -22,6 +22,10 @@
 # Test 'guix system', mostly error reporting.
 #
 
+# This test is expensive as it builds the images of multiple operating
+# systems; it takes more than a minute to run on a fast machine.
+test "$WITH_SLOW_TESTS" != 1 && exit 77
+
 set -e
 
 guix system --version
diff --git a/tests/guix-time-machine.sh b/tests/guix-time-machine.sh
index df75c681da..70a2443376 100644
--- a/tests/guix-time-machine.sh
+++ b/tests/guix-time-machine.sh
@@ -41,6 +41,8 @@ fi
 
 # Visiting a commit older than v0.16.0 must fail (this test is expensive
 # because it clones the whole repository).
-guix time-machine -q --commit=v0.15.0 $EXTRA_OPTIONS -- describe && false
+if [ "$WITH_SLOW_TESTS" = 1 ]; then
+    guix time-machine -q --commit=v0.15.0 $EXTRA_OPTIONS -- describe && false
+fi
 
 true
-- 
2.46.0

Message #11 received at 74394 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: 74394 <at> debbugs.gnu.org
Cc: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Subject: [PATCH 2/2] etc: Ensure test suite passes in pre-push git hook.
Date: Sun, 17 Nov 2024 21:21:47 +0900

* etc/git/pre-push: Run 'make check' on as many cores as available, which
takes about 1 minute on a fast machine.

Change-Id: I60e77d18bc885e8013daae70580126465169b1c6
---
 etc/git/pre-push | 1 +
 1 file changed, 1 insertion(+)

diff --git a/etc/git/pre-push b/etc/git/pre-push
index 325b23854b..74273ee180 100755
--- a/etc/git/pre-push
+++ b/etc/git/pre-push
@@ -33,6 +33,7 @@ do
 		case "$2" in
 		    *.gnu.org*)
 			set -e
+                        make check -j$(nproc)
 			make check-channel-news
 			exec guix git authenticate
 			exit 127
-- 
2.46.0

Message #14 received at 74394 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 74394 <at> debbugs.gnu.org
Subject: Re: [bug#74394] [PATCH 0/2] Skip slow tests by default and run
 'check' in Git pre-push hook.
Date: Fri, 29 Nov 2024 11:05:51 +0100

Hi Maxim,

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

> This is a simple change that should ensure test suite breakages are detected
> as early as possible and avoid tests breaking changes to be pushed.  This is
> made possible by skipping a few expensive tests suite, bringing down the total
> test time to about 1 minute on a fast machine.
>
> We could call it a "distributed CI" approach ;-).

I agree with the goal, of course, but not with the method: even without
expensive tests, “make check” is going to take maybe 5–10 minutes, and
having that happen when you run “git push” can be a terrible development
experience (especially since the developer most likely either already
ran the test suite or part of it right before, or pushes package changes
that have infinitely small probability of breaking “make check”).

Back to CI and not breaking things: I think that we should have a
workflow where the forge triggers those checks and puts a green light if
it passes, red light otherwise.  (Basically what everybody else is
doing. :-))

To me this should be one of the goals for the project in 2025.

> To run the complete test suite including the slow tests (as is the case prior
> this change):
>
> make check WITH_SLOW_TESTS=1

This variable itself may still be useful though (I’d call it
‘RUN_EXPENSIVE_TESTS’ or something like that—that’s the name used in
Coreutils—, “expensive” being the key word).  I would also keep it on by
default.

Thanks,
Ludo’.

Message #17 received at 74394 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 74394 <at> debbugs.gnu.org
Subject: Re: [bug#74394] [PATCH 0/2] Skip slow tests by default and run
 'check' in Git pre-push hook.
Date: Tue, 17 Dec 2024 09:28:59 +0900

Hi Ludovic,

Ludovic Courtès <ludo <at> gnu.org> writes:

> Hi Maxim,
>
> Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:
>
>> This is a simple change that should ensure test suite breakages are detected
>> as early as possible and avoid tests breaking changes to be pushed.  This is
>> made possible by skipping a few expensive tests suite, bringing down the total
>> test time to about 1 minute on a fast machine.
>>
>> We could call it a "distributed CI" approach ;-).
>
> I agree with the goal, of course, but not with the method: even without
> expensive tests, “make check” is going to take maybe 5–10 minutes, and
> having that happen when you run “git push” can be a terrible development
> experience (especially since the developer most likely either already
> ran the test suite or part of it right before, or pushes package changes
> that have infinitely small probability of breaking “make check”).

As I wrote, 'make check' with this change takes about 1 minute on my
machine; I'd be curious to know how long it takes on other people
machines; I suspect a bit more; if it's too slow, we can skip more, or
find out ways to make the tests run faster.

> Back to CI and not breaking things: I think that we should have a
> workflow where the forge triggers those checks and puts a green light if
> it passes, red light otherwise.  (Basically what everybody else is
> doing. :-))
>
> To me this should be one of the goals for the project in 2025.

That would be great.  I wonder how far QA is from making this
achievable.

>> To run the complete test suite including the slow tests (as is the case prior
>> this change):
>>
>> make check WITH_SLOW_TESTS=1
>
> This variable itself may still be useful though (I’d call it
> ‘RUN_EXPENSIVE_TESTS’ or something like that—that’s the name used in
> Coreutils—, “expensive” being the key word).  I would also keep it on by
> default.

One of the tests that was unbearably long when I measured was the
time-machine test.  It took about 20 minutes to fetch the git repository
with guile-git and run the test (which does extra work compared to the
CLI like validating each object).  I don't think we want this kind of
experience by default (because that'd probably convince people that
running the test suite often is not a reasonable thing to do).  The
other tests were more reasonable, with the longer ones in the 2-3
minutes range on my machine, IIRC.  Perhaps we could have this 20 minute
outlier skipped by default, maybe with a RUN_PROHIBITIVE_TESTS flag that
would default to 0 (false).

A long time ago I had read a blog post that argued that unit tests
should be small and fast [0], and there's a lot of good about that.
Fast tests usually translates in running the test suite more often and
catching breakage earlier.  As the author states, it also makes it
possible to determine whether a bug lies in the core logic or in the
integration of the many parts (unit tests vs integration tests), when
unit tests are decoupled from the whole system (typically by mocking all
external interfaces).

[0]  https://www.artima.com/weblogs/viewpost.jsp?thread=126923

-- 
Thanks,
Maxim

Message #20 received at 74394 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 74394 <at> debbugs.gnu.org
Subject: Re: [bug#74394] [PATCH 0/2] Skip slow tests by default and run
 'check' in Git pre-push hook.
Date: Tue, 17 Dec 2024 15:51:33 +0100

Hi Maxim,

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

> Ludovic Courtès <ludo <at> gnu.org> writes:

[...]

>> I agree with the goal, of course, but not with the method: even without
>> expensive tests, “make check” is going to take maybe 5–10 minutes, and
>> having that happen when you run “git push” can be a terrible development
>> experience (especially since the developer most likely either already
>> ran the test suite or part of it right before, or pushes package changes
>> that have infinitely small probability of breaking “make check”).
>
> As I wrote, 'make check' with this change takes about 1 minute on my
> machine;

Right now, without your patch, we have:

--8<---------------cut here---------------start------------->8---
$ wget -qO- $(guix build --log-file guix --no-grafts)|gunzip |grep "\`check'"
starting phase `check'
phase `check' succeeded after 2049.2 seconds
--8<---------------cut here---------------end--------------->8---

More than 30mn on the fast machines of the build farm, and with some of
the expensive tests already skipped (those that require network access:
time-machine, pack -RR, etc.).

This patch is not dividing wall-clock time by 30, is it?

>> This variable itself may still be useful though (I’d call it
>> ‘RUN_EXPENSIVE_TESTS’ or something like that—that’s the name used in
>> Coreutils—, “expensive” being the key word).  I would also keep it on by
>> default.
>
> One of the tests that was unbearably long when I measured was the
> time-machine test.  It took about 20 minutes to fetch the git repository
> with guile-git and run the test (which does extra work compared to the
> CLI like validating each object).  I don't think we want this kind of
> experience by default (because that'd probably convince people that
> running the test suite often is not a reasonable thing to do).  The
> other tests were more reasonable, with the longer ones in the 2-3
> minutes range on my machine, IIRC.  Perhaps we could have this 20 minute
> outlier skipped by default, maybe with a RUN_PROHIBITIVE_TESTS flag that
> would default to 0 (false).

Yeah okay, maybe we should skip them by default, and maybe we can find a
way to ensure developers do run them periodically.

> A long time ago I had read a blog post that argued that unit tests
> should be small and fast [0],

I actually agree.  :-)

Thanks!

Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.