Simple lisp-tests.el and commit privs

Simple lisp-tests.el and commit privs

[Barry OReilly]

[-- Attachment #1: Type: text/plain, Size: 273 bytes --]

Hi, I created some simple tests of the current Lisp indentation
behavior, attached.

I found the Emacs project on Savannah, but found no obvious way to
formally become a member through the website. I have a Savannah
account, public key registered, and the CA squared away.

[-- Attachment #2: lisp-tests.el --]
[-- Type: application/octet-stream, Size: 2209 bytes --]

;;; lisp-tests.el --- Test suite for lisp.el

;; Copyright (C) 2013 Free Software Foundation, Inc.

;; This file is part of GNU Emacs.

;; GNU Emacs is free software: you can redistribute it and/or modify
;; it under the terms of the GNU General Public License as published by
;; the Free Software Foundation, either version 3 of the License, or
;; (at your option) any later version.

;; GNU Emacs is distributed in the hope that it will be useful,
;; but WITHOUT ANY WARRANTY; without even the implied warranty of
;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
;; GNU General Public License for more details.

;; You should have received a copy of the GNU General Public License
;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.

;;; Commentary:

;;; Code:

(require 'ert)

(defmacro lisp-tests-with-temp-buffer (contents &rest body)
  "Create a `lisp-mode' enabled temp buffer with CONTENTS.
BODY is code to be executed within the temp buffer.  Point is
always located at the beginning of buffer."
  (declare (indent 1) (debug t))
  `(with-temp-buffer
     (emacs-lisp-mode)
     (insert ,contents)
     (goto-char (point-min))
     ,@body))

(defun lisp-tests-look-at (search-string)
  (re-search-forward search-string)
  (forward-char (- (length (match-string-no-properties 0)))))

(ert-deftest lisp-test-indent-line ()
  (lisp-tests-with-temp-buffer
   "
  (defun func ()
    (let ((x 10)
          ;; Next line at wrong indentation
    (y (some-func 10 20)))))
"
   (lisp-tests-look-at "(y ")
   (indent-for-tab-command)
   (should (eq 10 (current-indentation)))))

(ert-deftest lisp-test-indent-region ()
  (lisp-tests-with-temp-buffer
   "(defun func ()
  (let ((x 10)
        ;; Next line at wrong indentation
  (y (some-func 10
          20)))))
"
   (indent-region (point-min) (point-max))
   (lisp-tests-look-at "(defun func")
   (should (eq 0 (current-indentation)))
   (lisp-tests-look-at "(let ")
   (should (eq 2 (current-indentation)))
   (lisp-tests-look-at ";; ")
   (should (eq 8 (current-indentation)))
   (lisp-tests-look-at "(y ")
   (should (eq 8 (current-indentation)))
   (lisp-tests-look-at "20)")
   (should (eq 22 (current-indentation)))))

Re: Simple lisp-tests.el and commit privs

[Xue Fuqiao]

On Sun, Jul 21, 2013 at 12:12 PM, Barry OReilly <gundaetiapo@gmail.com> wrote:
> Hi, I created some simple tests of the current Lisp indentation
> behavior, attached.

Good, thank you!

> I found the Emacs project on Savannah, but found no obvious way to
> formally become a member through the website. I have a Savannah
> account, public key registered, and the CA squared away.

In your Group Membership page[fn:1], you can see a "Request for
Inclusion".  Type below "emacs", write your comments, then submit.

Footnotes:

[fn:1] https://savannah.gnu.org/my/groups.php

--
Best regards, Xue Fuqiao.
http://www.gnu.org/software/emacs/

Re: Simple lisp-tests.el and commit privs

[Stefan Monnier]

> Hi, I created some simple tests of the current Lisp indentation
> behavior, attached.

Currently, the way I like to handle indentation tests is by adding files
in test/indent/ where the file simply contains a pre-indented content,
and the Makefile lets you "reindent, then run diff to see if something
changed".

Simple and effective.


        Stefan

Re: Simple lisp-tests.el and commit privs

[Barry OReilly]

[-- Attachment #1: Type: text/plain, Size: 933 bytes --]

Stefan:
> Currently, the way I like to handle indentation tests is by adding
> files in test/indent/ where the file simply contains a pre-indented
> content, and the Makefile lets you "reindent, then run diff to see
> if something changed".

I couldn't tell, since test/automated has ERT indentation tests for
Python, Ruby, and Fortran. I didn't find that your preferred
diff based indentation tests are run as a part of the automated 'make
check'. I did find several fail when run manually.

Automating the indentation tests would make them more valuable. If we
were to automate the diff based indentation tests, I think we ought to
wrap it with ERT so as it is executed and reported in the same manner
as the other automated tests. Yet if we're going to use ERT, then the
approach taken by the python-tests.el, ruby-mode-tests.el, f90.el
isn't less simple.

Finally, why foreclose tests for indenting non indented buffer
contents?

[-- Attachment #2: Type: text/html, Size: 1074 bytes --]

Re: Simple lisp-tests.el and commit privs

[Dmitry Gutov]

Barry OReilly <gundaetiapo@gmail.com> writes:

> Stefan:
>> Currently, the way I like to handle indentation tests is by adding
>> files in test/indent/ where the file simply contains a pre-indented
>> content, and the Makefile lets you "reindent, then run diff to see
>> if something changed".
>
> I couldn't tell, since test/automated has ERT indentation tests for
> Python, Ruby, and Fortran.

Those are written by people other than Stefan, e.g. me.

There was a relevant discussion not too long ago, in emacs-diffs (sorry,
my fault):
http://lists.gnu.org/archive/html/emacs-diffs/2013-05/msg00275.html

It concluded with a corollary to use the files in test/indent for simple
indentation tests, and ERT for more complex stuff.

Yet, as reluctant as I was to disagree, I couldn't eventually bring
myself to take the [separate, uniquely named] indentation tests from
ruby-mode-test.el and lump them together, so to speak, into ruby.rb.

So, as far as I'm concerned, a better solution would be to extend ERT
runner with a command that would pop a buffer in which a test had
failed, so that the programmer can "try out sexp movement and
indentation functions "in the usual way", together with Edebug if
needed", as per Stefan's explanation.

Re: Simple lisp-tests.el and commit privs

[Barry OReilly]

[-- Attachment #1: Type: text/plain, Size: 985 bytes --]

Thanks for the link to the previous discussion, Dmitry.

Dmitry:
>> Don't you think it would be better to have these tests in
>> test/automated/ruby-mode-tests.el?

Stefan:
> I find it inconvenient, actually, because when the test fails, it's
> a handy to be able to have a ruby-mode buffer where I can try out
> sexp movement and indentation functions "in the usual way", together
> with Edebug if needed.

Surely there's a way to provide this debugging use case without
ostracizing the test from the automated test suite. Why not have a
ert-with-temp-buffer which if the test fails (within the form), a file
would be saved off with the buffer contents. Ideally the test case
name would be in the generated filename.

Stefan:
> use test/indent the general indentation rules, and use ERT rules to
> check how indentation obeys the various indentation variables

It looks like the test/indent tests are only good for null-diff
general indentation rules. And worst of all not automated.

[-- Attachment #2: Type: text/html, Size: 1148 bytes --]

Re: Simple lisp-tests.el and commit privs

[Stefan Monnier]

> It looks like the test/indent tests are only good for null-diff
> general indentation rules. And worst of all not automated.

Given that it requires 0 amount of work to set it up and debug it, I say
this is great bang for your buck.


        Stefan

Re: Simple lisp-tests.el and commit privs

[Stefan Monnier]

> It looks like the test/indent tests are only good for null-diff
> general indentation rules.

What makes you think so?  Nothing prevents you from stripping indentation
on every line and then passing the result to indent-region.

> And worst of all not automated.

Actually, it's partly automated.

But that can be automated further, including running it via ERT.
Patches welcome,


        Stefan

Re: Simple lisp-tests.el and commit privs

[Barry OReilly]

[-- Attachment #1: Type: text/plain, Size: 407 bytes --]

>> It looks like the test/indent tests are only good for null-diff
>> general indentation rules.

> What makes you think so?

When the diff outputs differences (ie a non null-diff), how does the
current test/indent distinguish expected from unexpected? If there
was a test/indent/ruby.rb.expected file (for example), then I could
see how it could do so: 'diff ruby.rb.expected ruby.rb.new' returns
nothing.

[-- Attachment #2: Type: text/html, Size: 488 bytes --]

Re: Simple lisp-tests.el and commit privs

[Stefan Monnier]

>>> It looks like the test/indent tests are only good for null-diff
>>> general indentation rules.
>> What makes you think so?
> When the diff outputs differences (ie a non null-diff), how does the
> current test/indent distinguish expected from unexpected? If there
> was a test/indent/ruby.rb.expected file (for example), then I could
> see how it could do so: 'diff ruby.rb.expected ruby.rb.new' returns
> nothing.

Oh, you're referring to known indentation problems, where you want to
have the test but have it be marked as "expected".

Yes, that's currently not really supported: I try to put "FIXME" on
the corresponding lines, but very often an incorrect indentation on one
line impact subsequent lines as well, so not only does the "diff" not
care about those FIXME markers, but you can get false positives or false
negatives to boot.


        Stefan

Re: Simple lisp-tests.el and commit privs

[Barry OReilly]

[-- Attachment #1: Type: text/plain, Size: 1629 bytes --]

> Oh, you're referring to known indentation problems, where you want
> to have the test but have it be marked as "expected".

Not just that. Suppose ruby.rb contains:

  if a == 2
    puts "hello"
    else
  puts "there"
  end

so as we can test general indentation which changes the buffer.

Expected success (current behavior):

--- ruby.rb     2013-08-20 11:29:39.000000000 -0400
+++ ruby.rb.new 2013-08-20 11:29:42.000000000 -0400
@@ -35,8 +35,8 @@

   if a == 2
     puts "hello"
-    else
-  puts "there"
+  else
+    puts "there"
   end

   if a == 2 then

Unexpected failure (if "else" didn't move -- is not current behavior):

   if a == 2
     puts "hello"
     else
-  puts "there"
+    puts "there"
   end

   if a == 2 then

Expected failure (current behavior)

@@ -52,11 +52,11 @@
   case a
   when "a"
     6
-  # Support for this syntax was removed in Ruby 1.9, so we
-  # probably don't need to handle it either.
-  # when "b" :
-  #   7
-  # when "c" : 2
+    # Support for this syntax was removed in Ruby 1.9, so we
+    # probably don't need to handle it either.
+    # when "b" :
+    #   7
+    # when "c" : 2
   when "d"  then 4
   else 5
   end

ERT has a means of distinguishing these. However, if we lump all these
in one test/indent/ruby.rb file and do as 'make ruby.rb.test' from one
ERT test, then these aren't distinguishable (in a simple way).

> Why not have a ert-with-temp-buffer which if the test fails (within
> the form), a file would be saved off with the buffer contents.
> Ideally the test case name would be in the generated filename.

Is this inadequate to address your debugging needs?

[-- Attachment #2: Type: text/html, Size: 2085 bytes --]

Re: Simple lisp-tests.el and commit privs

[Stefan Monnier]

> Expected success (current behavior):

> --- ruby.rb     2013-08-20 11:29:39.000000000 -0400
> +++ ruby.rb.new 2013-08-20 11:29:42.000000000 -0400
> @@ -35,8 +35,8 @@

>    if a == 2
>      puts "hello"
> -    else
> -  puts "there"
> +  else
> +    puts "there"
>    end

>    if a == 2 then

> Unexpected failure (if "else" didn't move -- is not current behavior):

>    if a == 2
>      puts "hello"
>      else
> -  puts "there"
> +    puts "there"
>    end

>    if a == 2 then

I don't see what's the problem here: if indent/ruby.rb contains:

  if a == 2
    puts "hello"
  else
    puts "there"
    puts "there"
  end

then presumably the bug would get caught.  You may have to run the test
by first flushing all the code to the left margin before reindenting,
but if reindentation works correctly both when started "at left-margin"
and when started "at the correct indentation", then there's a very high
probability that it will work in most/all other cases as well.

Do you know of bugs that have occurred and that wouldn't be caught this way?
  
>> Why not have a ert-with-temp-buffer which if the test fails (within
>> the form), a file would be saved off with the buffer contents.
>> Ideally the test case name would be in the generated filename.
> Is this inadequate to address your debugging needs?

I still don't see what problem this is trying to address.


        Stefan

Re: Simple lisp-tests.el and commit privs

[Barry OReilly]

[-- Attachment #1: Type: text/plain, Size: 1207 bytes --]

> You may have to run the test by first flushing all the code to the
> left margin before reindenting

That would certainly make the tests better. Only testing preindented
buffers is good but insufficient. What you describe is a good way to
test non preindented buffers.

> Oh, you're referring to known indentation problems, where you want to
> have the test but have it be marked as "expected".
>
> Yes, that's currently not really supported: I try to put "FIXME" on
> the corresponding lines, but very often an incorrect indentation on
> one line impact subsequent lines as well, so not only does the
> "diff" not care about those FIXME markers, but you can get false
> positives or false negatives to boot.

Sounds more complex than using ERT :expected-result.

If I indicate :expected-result is :failed on an invocation of a
test/indent test from ERT, it would suppress all the expected
successes because of one expected failure. Alternatively, the test
could filter out lines with "FIXME", but that's ugly.

I think finer grained ERT tests that can be marked as :expected-result
of :passed or :failed (and don't need to parse for FIXMEs) are the
better approach for test/automated 'make check' tests.

[-- Attachment #2: Type: text/html, Size: 1416 bytes --]