GNU bug report logs - #36452
[PATCH] Add tests for bookmark.el

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 36452 in the body.
You can then email your comments to 36452 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 <stefan <at> marxist.se>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Add tests for bookmark.el
Date: Mon, 1 Jul 2019 01:32:56 +0200

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

I've written tests for bookmark.el.  Please see the attached patch.

I did not write tests for bookmark-bmenu at this time, but I'm
considering doing that next.  I anyway figured it's better that this
gets reviewed and installed before it gets any longer.

Comments and suggestions are very welcome.

Thanks,
Stefan Kangas

[0001-Add-tests-for-bookmark.el.patch (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 36452 <at> debbugs.gnu.org
Subject: Re: bug#36452: [PATCH] Add tests for bookmark.el
Date: Tue, 02 Jul 2019 19:41:00 +0300

> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Mon, 1 Jul 2019 01:32:56 +0200
> 
> I've written tests for bookmark.el.  Please see the attached patch.

Thanks.  Can you tell the approximate coverage of bookmark
functionality by these tests?

One particular aspect of the bookmarks that I'd like to see tested is
non-ASCII bookmark names and annotations.  The code includes some
slightly involved logic for determining how to encode the info on
disk, so I wonder how well does that work in practice.

> --- /dev/null
> +++ b/test/lisp/bookmark-resources/test.bmk
> @@ -0,0 +1,10 @@
> +;;;; Emacs Bookmark Format Version 1 ;;;; -*- coding: utf-8-emacs -*-
> +;;; This format is meant to be slightly human-readable;
> +;;; nevertheless, you probably don't want to edit it.
> +;;; -*- End Of Bookmark File Format Version Stamp -*-
> +(("name"
> + (filename . "/some/file")
> + (front-context-string . "abc")
> + (rear-context-string . "def")
> + (position . 3))
> +)
> \ No newline at end of file

Is this lack of newline intentional?

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

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 36452 <at> debbugs.gnu.org
Subject: Re: bug#36452: [PATCH] Add tests for bookmark.el
Date: Wed, 3 Jul 2019 08:33:22 +0200

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

Eli Zaretskii <eliz <at> gnu.org> writes:

> Thanks.  Can you tell the approximate coverage of bookmark
> functionality by these tests?

All core functionality is tested.  I didn't test interactive stuff
like bookmark-completing-read, nor did I bother testing the ancient
code that upgrades from the pre-1995 bookmark format.  Other than
that, I would say that it's comprehensive.

Running the tests it with M-x testcover-* unfortunately does not give
an exact figure, but I don't see any important code paths that have
been missed.

> One particular aspect of the bookmarks that I'd like to see tested is
> non-ASCII bookmark names and annotations.  The code includes some
> slightly involved logic for determining how to encode the info on
> disk, so I wonder how well does that work in practice.

OK.  I added tests for that in the attached patch using strings
manually copied from HELLO.  It works just fine to save and then load
bookmarks for the set of languages I chose to include characters from.
That is, we have the same before bookmark-save as we get after
bookmark-load.

> > \ No newline at end of file
>
> Is this lack of newline intentional?

Yes.  This is how this file is written by `bookmark-write-file'.
However, I think it would be nice if it did add the newline (e.g. for
files under VC), so I changed that here.

Thank you for your review.  Please find attached an updated version of
the patch.

Best regards,
Stefan Kangas

[0001-Add-tests-for-bookmark.el-Bug-36452.patch (application/octet-stream, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 36452 <at> debbugs.gnu.org
Subject: Re: bug#36452: [PATCH] Add tests for bookmark.el
Date: Wed, 03 Jul 2019 09:59:54 +0300

> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Wed, 3 Jul 2019 08:33:22 +0200
> Cc: 36452 <at> debbugs.gnu.org
> 
> > One particular aspect of the bookmarks that I'd like to see tested is
> > non-ASCII bookmark names and annotations.  The code includes some
> > slightly involved logic for determining how to encode the info on
> > disk, so I wonder how well does that work in practice.
> 
> OK.  I added tests for that in the attached patch using strings
> manually copied from HELLO.  It works just fine to save and then load
> bookmarks for the set of languages I chose to include characters from.
> That is, we have the same before bookmark-save as we get after
> bookmark-load.

Thanks.  Please add to that text something that doesn't yield valid
UTF-8 byte sequence.  For example, these two strings:

  (decode-coding-string "\xE3\x32\x9A\x36" 'chinese-gb18030)
  (char-to-string (decode-char 'eight-bit #x81))

(Please include these snippets directly, not their result as a literal
string, to make sure the unusual characters survive intact.)

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

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 36452 <at> debbugs.gnu.org
Subject: Re: bug#36452: [PATCH] Add tests for bookmark.el
Date: Wed, 3 Jul 2019 13:01:13 +0200

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

Eli Zaretskii <eliz <at> gnu.org> writes:
> > > One particular aspect of the bookmarks that I'd like to see tested is
> > > non-ASCII bookmark names and annotations.  The code includes some
> > > slightly involved logic for determining how to encode the info on
> > > disk, so I wonder how well does that work in practice.
> >
> > OK.  I added tests for that in the attached patch using strings
> > manually copied from HELLO.  It works just fine to save and then load
> > bookmarks for the set of languages I chose to include characters from.
> > That is, we have the same before bookmark-save as we get after
> > bookmark-load.
>
> Thanks.  Please add to that text something that doesn't yield valid
> UTF-8 byte sequence.  For example, these two strings:
>
>   (decode-coding-string "\xE3\x32\x9A\x36" 'chinese-gb18030)
>   (char-to-string (decode-char 'eight-bit #x81))
>
> (Please include these snippets directly, not their result as a literal
> string, to make sure the unusual characters survive intact.)

OK, added.  The tests still pass (both interactively and in batch mode).

The first one, when I evaluate it, makes Emacs segfault.  Not sure why
it works when I'm running the tests - I'd guess because Emacs is not
trying to display it?  Let me know if you want a backtrace for this
crash and/or a separate bug report.

In any case, I've attached an updated patch with the two above
snippets included in the save/load test.  Not sure if we should keep
the first one around, but I added a warning not to evaluate it.

What do you think?

Thanks,
Stefan Kangas

[0001-Add-tests-for-bookmark.el-Bug-36452.patch (application/octet-stream, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 36452 <at> debbugs.gnu.org
Subject: Re: bug#36452: [PATCH] Add tests for bookmark.el
Date: Wed, 03 Jul 2019 14:24:56 +0300

> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Wed, 3 Jul 2019 13:01:13 +0200
> Cc: 36452 <at> debbugs.gnu.org
> 
> > Thanks.  Please add to that text something that doesn't yield valid
> > UTF-8 byte sequence.  For example, these two strings:
> >
> >   (decode-coding-string "\xE3\x32\x9A\x36" 'chinese-gb18030)
> >   (char-to-string (decode-char 'eight-bit #x81))
> >
> > (Please include these snippets directly, not their result as a literal
> > string, to make sure the unusual characters survive intact.)
> 
> OK, added.  The tests still pass (both interactively and in batch mode).

Great, thanks.

> The first one, when I evaluate it, makes Emacs segfault.  Not sure why
> it works when I'm running the tests - I'd guess because Emacs is not
> trying to display it?  Let me know if you want a backtrace for this
> crash and/or a separate bug report.

A separate bug report, please.  It didn't crash for me, FWIW.  If the
crash is in the display code, then yes, it's because evaluating the
expression will display the resulting string.

> In any case, I've attached an updated patch with the two above
> snippets included in the save/load test.  Not sure if we should keep
> the first one around, but I added a warning not to evaluate it.
> 
> What do you think?

I think we should keep both, but we should also investigate the crash.

Do the tests in this patch depend in some way on the other patches you
submitted for bookmark.el?  Because if not, I'd like to push this
right away, as tests cannot possibly break anything (with the other
patches, I'd like to wait for comments for a few more days).

Thanks.

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

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 36452 <at> debbugs.gnu.org
Subject: Re: bug#36452: [PATCH] Add tests for bookmark.el
Date: Wed, 3 Jul 2019 13:42:24 +0200

Eli Zaretskii <eliz <at> gnu.org> writes:
> > The first one, when I evaluate it, makes Emacs segfault.  Not sure why
> > it works when I'm running the tests - I'd guess because Emacs is not
> > trying to display it?  Let me know if you want a backtrace for this
> > crash and/or a separate bug report.
>
> A separate bug report, please.  It didn't crash for me, FWIW.  If the
> crash is in the display code, then yes, it's because evaluating the
> expression will display the resulting string.

OK, I'll submit a separate bug report.

> I think we should keep both, but we should also investigate the crash.

Sounds good.

> Do the tests in this patch depend in some way on the other patches you
> submitted for bookmark.el?  Because if not, I'd like to push this
> right away, as tests cannot possibly break anything (with the other
> patches, I'd like to wait for comments for a few more days).

This patch is fully independent from the others.  It would be great if
it could be installed right away.

Thanks,
Stefan Kangas

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 36452-done <at> debbugs.gnu.org
Subject: Re: bug#36452: [PATCH] Add tests for bookmark.el
Date: Wed, 03 Jul 2019 14:53:05 +0300

> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Wed, 3 Jul 2019 13:42:24 +0200
> Cc: 36452 <at> debbugs.gnu.org
> 
> > Do the tests in this patch depend in some way on the other patches you
> > submitted for bookmark.el?  Because if not, I'd like to push this
> > right away, as tests cannot possibly break anything (with the other
> > patches, I'd like to wait for comments for a few more days).
> 
> This patch is fully independent from the others.  It would be great if
> it could be installed right away.

Thanks, pushed.

Previous Next

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