Subject: Re: bug#76839: [Roi Martin] [PATCH] Fix 'Skip' behavior in erts files

From: Stefan Kangas <[email-masked]>
Date: Fri, 7 Mar 2025 14:18:20 -0800

Roi Martin <[email-masked]> writes:

> This is a second version of the patch that adds tests for this bug.  I
> also rebased it onto master.
>
> From 870eed35b5b1fa20a5e038abb8bf32827e56fefa Mon Sep 17 00:00:00 2001
> From: Roi Martin <[email-masked]>
> Date: Thu, 6 Mar 2025 20:26:46 +0100
> Subject: [PATCH] Fix 'Skip' behavior in erts files (Bug#76839)
>
> * lisp/emacs-lisp/ert.el (ert-test--erts-test): Fix 'Skip' behavior in
> erts files, so only the test case where it is specified is skipped.
> * test/lisp/emacs-lisp/ert-tests.el (ert-test-erts-skip-one)
> (ert-test-erts-skip-last): Add test cases.
> ---
>  lisp/emacs-lisp/ert.el                           |  2 +-
>  .../emacs-lisp/ert-resources/erts-skip-last.erts |  8 ++++++++
>  .../emacs-lisp/ert-resources/erts-skip-one.erts  | 16 ++++++++++++++++
>  test/lisp/emacs-lisp/ert-tests.el                | 12 ++++++++++++
>  4 files changed, 37 insertions(+), 1 deletion(-)
>  create mode 100644 test/lisp/emacs-lisp/ert-resources/erts-skip-last.erts
>  create mode 100644 test/lisp/emacs-lisp/ert-resources/erts-skip-one.erts
>
> diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
> index c57bd0a69e2f..eedff982940f 100644
> --- a/lisp/emacs-lisp/ert.el
> +++ b/lisp/emacs-lisp/ert.el
> @@ -2901,7 +2901,7 @@ ert-test--erts-test
>      (if (and skip
>               (eval (car (read-from-string skip))))
>          ;; Skipping this test.
> -        ()
> +        (goto-char end-after)
>        ;; Do the test.
>        (goto-char end-after)
>        ;; We have a separate after section.
> diff --git a/test/lisp/emacs-lisp/ert-resources/erts-skip-last.erts b/test/lisp/emacs-lisp/ert-resources/erts-skip-last.erts
> new file mode 100644
> index 000000000000..fd39efcaaa6c
> --- /dev/null
> +++ b/test/lisp/emacs-lisp/ert-resources/erts-skip-last.erts
> @@ -0,0 +1,8 @@
> +Name: last
> +Skip: t
> +
> +=-=
> +FOO
> +=-=
> +BAR
> +=-=-=
> diff --git a/test/lisp/emacs-lisp/ert-resources/erts-skip-one.erts b/test/lisp/emacs-lisp/ert-resources/erts-skip-one.erts
> new file mode 100644
> index 000000000000..3b35081c4148
> --- /dev/null
> +++ b/test/lisp/emacs-lisp/ert-resources/erts-skip-one.erts
> @@ -0,0 +1,16 @@
> +Name: first
> +Skip: t
> +
> +=-=
> +FOO
> +=-=
> +FOO
> +=-=-=
> +
> +Name: second
> +
> +=-=
> +FOO
> +=-=
> +BAR
> +=-=-=
> diff --git a/test/lisp/emacs-lisp/ert-tests.el b/test/lisp/emacs-lisp/ert-tests.el
> index aec2c92ba811..85b94c8d1a1e 100644
> --- a/test/lisp/emacs-lisp/ert-tests.el
> +++ b/test/lisp/emacs-lisp/ert-tests.el
> @@ -28,6 +28,7 @@
>
>  (require 'cl-lib)
>  (require 'ert)
> +(require 'ert-x)
>
>  ;;; Self-test that doesn't rely on ERT, for bootstrapping.
>
> @@ -1021,6 +1022,17 @@ ert-test-with-test-buffer-selected/buffer-name
>                   (ert-with-test-buffer (:name "foo" :selected t)
>                     (buffer-name)))))
>
> +(ert-deftest ert-test-erts-skip-one ()
> +  "Test that Skip does not affect subsequent test cases (Bug#76839)."
> +  :expected-result :failed

I think this should be avoided, because a test can fail for any reason.

Can we turn the condition around, so that this test always passes?

> +  (ert-test-erts-file (ert-resource-file "erts-skip-one.erts")
> +		      (lambda () ())))
> +
> +(ert-deftest ert-test-erts-skip-last ()
> +  "Test that Skip does not fail on last test case (Bug#76839)."
> +  (ert-test-erts-file (ert-resource-file "erts-skip-last.erts")
> +		      (lambda () ())))
> +
>  (provide 'ert-tests)
>
>  ;;; ert-tests.el ends here
> --
> 2.48.1