GNU bug report logs - #76839
[PATCH] Fix 'Skip' behavior in erts files

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 76839 in the body.
You can then email your comments to 76839 AT debbugs.gnu.org in the normal way.

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

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [Roi Martin] [PATCH] Fix 'Skip' behavior in erts files
Date: Fri, 7 Mar 2025 19:37:30 +0000

[Message part 1 (text/plain, inline)]

Forwarding this so that we don't lose track of it.

-------------------- Start of forwarded message --------------------
From: Roi Martin <jroi.martin <at> gmail.com>
To: emacs-devel <at> gnu.org
Subject: [PATCH] Fix 'Skip' behavior in erts files
Date: Thu, 06 Mar 2025 21:06:24 +0100

[Message part 2 (text/plain, attachment)]

[0001-Fix-Skip-behavior-in-erts-files.patch (text/plain, attachment)]

[Message part 4 (text/plain, attachment)]

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

From: Roi Martin <jroi.martin <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>, 76839 <at> debbugs.gnu.org
Subject: Re: bug#76839: [Roi Martin] [PATCH] Fix 'Skip' behavior in erts files
Date: Fri, 07 Mar 2025 23:08:54 +0100

[Message part 1 (text/plain, inline)]

This is a second version of the patch that adds tests for this bug.  I
also rebased it onto master.

[0001-Fix-Skip-behavior-in-erts-files-Bug-76839.patch (text/plain, attachment)]

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Roi Martin <jroi.martin <at> gmail.com>, 76839 <at> debbugs.gnu.org
Subject: Re: bug#76839: [Roi Martin] [PATCH] Fix 'Skip' behavior in erts files
Date: Fri, 7 Mar 2025 14:18:20 -0800

Roi Martin <jroi.martin <at> gmail.com> 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 <jroi.martin <at> gmail.com>
> 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

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

From: Roi Martin <jroi.martin <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>, 76839 <at> debbugs.gnu.org
Subject: Re: bug#76839: [Roi Martin] [PATCH] Fix 'Skip' behavior in erts files
Date: Sat, 08 Mar 2025 00:21:17 +0100

Stefan Kangas <stefankangas <at> gmail.com> writes:

>> +(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?

The thing is that I need to determine that the test case following the
skipped one in the erts file has been executed.  If I know that the test
will fail, it is easy to identify that, which is why I use
':expected-result :failed'.  However, If we expect `ert-test-erts-file'
to succeed, how can I confirm that the second test case has passed
instead of being skipped as well?

I guess I could handle the `ert-test-failed' error signaled by
`ert-test-erts-file' when it calls `ert-fail', but is it much better?
Are there any other alternatives?

Thanks!

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Roi Martin <jroi.martin <at> gmail.com>, 76839 <at> debbugs.gnu.org
Subject: Re: bug#76839: [Roi Martin] [PATCH] Fix 'Skip' behavior in erts files
Date: Sat, 8 Mar 2025 09:57:04 +0000

Roi Martin <jroi.martin <at> gmail.com> writes:

> Stefan Kangas <stefankangas <at> gmail.com> writes:
>
>>> +(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?
>
> The thing is that I need to determine that the test case following the
> skipped one in the erts file has been executed.  If I know that the test
> will fail, it is easy to identify that, which is why I use
> ':expected-result :failed'.  However, If we expect `ert-test-erts-file'
> to succeed, how can I confirm that the second test case has passed
> instead of being skipped as well?
>
> I guess I could handle the `ert-test-failed' error signaled by
> `ert-test-erts-file' when it calls `ert-fail', but is it much better?

I think it's better, because then you reduce the scope to just that one
error.

The test could also fail for any other reason, including that some form
couldn't be read, an incorrect erts file, and so on.  We would want such
things to be flagged, in case they happen.

Does that make sense?  Could you propose an updated patch along these
lines?

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

From: Roi Martin <jroi.martin <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>, 76839 <at> debbugs.gnu.org
Subject: Re: bug#76839: [Roi Martin] [PATCH] Fix 'Skip' behavior in erts files
Date: Sat, 08 Mar 2025 20:37:20 +0100

[Message part 1 (text/plain, inline)]

Stefan Kangas <stefankangas <at> gmail.com> writes:

>> I guess I could handle the `ert-test-failed' error signaled by
>> `ert-test-erts-file' when it calls `ert-fail', but is it much better?
>
> I think it's better, because then you reduce the scope to just that one
> error.
>
> The test could also fail for any other reason, including that some form
> couldn't be read, an incorrect erts file, and so on.  We would want such
> things to be flagged, in case they happen.
>
> Does that make sense?  Could you propose an updated patch along these
> lines?

Yes, it makes sense.  I attached a new version that implements your
suggestions.

[0001-Fix-Skip-behavior-in-erts-files-Bug-76839.patch (text/plain, attachment)]

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

From: Roi Martin <jroi.martin <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>, 76839 <at> debbugs.gnu.org
Subject: Re: bug#76839: [Roi Martin] [PATCH] Fix 'Skip' behavior in erts files
Date: Mon, 17 Mar 2025 19:32:02 +0100

Do you think we can merge this?  It is a bug fix and being so small I
guess it is within the limits of a contribution that do not require CA.

In any case, my CA paperwork is in progress but it seems that it will
take a bit more time.

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Roi Martin <jroi.martin <at> gmail.com>, 76839 <at> debbugs.gnu.org
Subject: Re: bug#76839: [Roi Martin] [PATCH] Fix 'Skip' behavior in erts files
Date: Sat, 22 Mar 2025 11:17:55 +0000

Roi Martin <jroi.martin <at> gmail.com> writes:

> Do you think we can merge this?  It is a bug fix and being so small I
> guess it is within the limits of a contribution that do not require CA.
>
> In any case, my CA paperwork is in progress but it seems that it will
> take a bit more time.

Unfortunately, I think we have to wait for the paperwork to come through
here.  Roughly speaking, any contribution above 15 lines or so counts as
significant enough to require a copyright assignment.

Please feel free to ping again when you have confirmation that the
papers are done.

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

From: Roi Martin <jroi.martin <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>, 76839 <at> debbugs.gnu.org
Subject: Re: bug#76839: [Roi Martin] [PATCH] Fix 'Skip' behavior in erts files
Date: Fri, 18 Apr 2025 16:33:38 +0200

[Message part 1 (text/plain, inline)]

Stefan Kangas <stefankangas <at> gmail.com> writes:

> Unfortunately, I think we have to wait for the paperwork to come through
> here.  Roughly speaking, any contribution above 15 lines or so counts as
> significant enough to require a copyright assignment.

My CA paperwork is now completed.

I attached a new version of the patch rebased against current master
(9f0c43a3d1b3 "; * etc/NEWS: Copyediting.").

[0001-Fix-Skip-behavior-in-erts-files-Bug-76839.patch (text/x-patch, attachment)]

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

From: Roi Martin <jroi.martin <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>, 76839 <at> debbugs.gnu.org
Subject: Re: bug#76839: [Roi Martin] [PATCH] Fix 'Skip' behavior in erts files
Date: Mon, 28 Apr 2025 07:13:06 +0200

Roi Martin <jroi.martin <at> gmail.com> writes:

> My CA paperwork is now completed.
>
> I attached a new version of the patch rebased against current master
> (9f0c43a3d1b3 "; * etc/NEWS: Copyediting.").

Are any additional changes needed to install this patch?

Message #39 received at 76839-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Roi Martin <jroi.martin <at> gmail.com>
Cc: stefankangas <at> gmail.com, 76839-done <at> debbugs.gnu.org
Subject: Re: bug#76839: [Roi Martin] [PATCH] Fix 'Skip' behavior in erts files
Date: Sat, 03 May 2025 10:32:55 +0300

> From: Roi Martin <jroi.martin <at> gmail.com>
> Date: Mon, 28 Apr 2025 07:13:06 +0200
> 
> Roi Martin <jroi.martin <at> gmail.com> writes:
> 
> > My CA paperwork is now completed.
> >
> > I attached a new version of the patch rebased against current master
> > (9f0c43a3d1b3 "; * etc/NEWS: Copyediting.").
> 
> Are any additional changes needed to install this patch?

Thanks, I've now installed this on the master branch, and I'm closing
this bug.

Previous Next

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