moving some indentation tests

moving some indentation tests

[Tom Tromey]

Currently indentation tests seem to be in test/manual/indent.  However,
most of these could be run automatically.  So, I'd like to move them so
that they'll be run by the ordinary "make check".

I'm thinking I would move the files to test/data and then write a new
test that simply iterates over these files and checking that
re-indentation works.

Not all the files can be moved this way -- a few currently fail.  I
wouldn't move those.

Let me know what you think.

Tom

Re: moving some indentation tests

[Michael Albinus]

Tom Tromey <tom@tromey.com> writes:

> I'm thinking I would move the files to test/data and then write a new
> test that simply iterates over these files and checking that
> re-indentation works.

test/file-organization.org makes another proposal. You shall create a
FEATURE-resources directory, given the test file is called FEATURE-tests.el.

> Tom

Best regards, Michael.

Re: moving some indentation tests

[Eli Zaretskii]

> From: Tom Tromey <tom@tromey.com>
> Date: Sun, 02 Apr 2017 03:47:26 -0600
> 
> Currently indentation tests seem to be in test/manual/indent.  However,
> most of these could be run automatically.  So, I'd like to move them so
> that they'll be run by the ordinary "make check".
> 
> I'm thinking I would move the files to test/data and then write a new
> test that simply iterates over these files and checking that
> re-indentation works.
> 
> Not all the files can be moved this way -- a few currently fail.  I
> wouldn't move those.
> 
> Let me know what you think.

Sounds good, but please use the convention pointed out by Michael
regarding the data files.

Thanks.

Re: moving some indentation tests

[Dmitry Gutov]

On 02.04.2017 12:47, Tom Tromey wrote:

> Not all the files can be moved this way -- a few currently fail.  I
> wouldn't move those.

I'd rather avoid splitting them. Do we have a lot of failures, and would 
it be hard to try to appoint people responsible for them?

Re: moving some indentation tests

[Tom Tromey]

>>>>> "Dmitry" == Dmitry Gutov <dgutov@yandex.ru> writes:

Dmitry> On 02.04.2017 12:47, Tom Tromey wrote:
>> Not all the files can be moved this way -- a few currently fail.  I
>> wouldn't move those.

Dmitry> I'd rather avoid splitting them. Do we have a lot of failures, and
Dmitry> would it be hard to try to appoint people responsible for them?

Here are the failures.

5 matches for "failed" in buffer: Log
     41:Makefile:13: recipe for target 'octave.m.test' failed
   1947:Makefile:13: recipe for target 'pascal.pas.test' failed
   2037:Makefile:13: recipe for target 'perl.perl.test' failed
   2072:Makefile:13: recipe for target 'scheme.scm.test' failed
   2097:Makefile:13: recipe for target 'shell.rc.test' failed


It's simple to move remaining ones if they are fixed.
I think it's preferable by far, though, not to wait, unless the fixes
are imminent.

Tom

Re: moving some indentation tests

[Tom Tromey]

>>>>> "Michael" == Michael Albinus <michael.albinus@gmx.de> writes:

Michael> test/file-organization.org makes another proposal. You shall
Michael> create a FEATURE-resources directory, given the test file is
Michael> called FEATURE-tests.el.

Alright.
I saw this file but since test/data exists and isn't documented, I
assumed it was out of date.

Tom

Re: moving some indentation tests

[Michael Albinus]

Tom Tromey <tom@tromey.com> writes:

> I saw this file but since test/data exists and isn't documented, I
> assumed it was out of date.

I believe test/data is rather out of date. Maybe we shall rearrange it?

> Tom

Best regards, Michael.

Re: moving some indentation tests

[Phillip Lord]

Tom Tromey <tom@tromey.com> writes:

> Currently indentation tests seem to be in test/manual/indent.  However,
> most of these could be run automatically.  So, I'd like to move them so
> that they'll be run by the ordinary "make check".
>
> I'm thinking I would move the files to test/data and then write a new
> test that simply iterates over these files and checking that
> re-indentation works.

My own package, assess, provides good support for this kind of
functionality. In particular, you can do this:


(assess-roundtrip-indentation=
 'emacs-lisp-mode
 "
(assess-with-find-file
    \"~/.emacs\"
  (buffer-string))")
  
if you want to see what you are indenting. And, 

(assess-file-roundtrip-indentation=
  "assess.el")

if you want to use a file.

I've been planning to add this to core -- I was hoping to wait till we
have a "core ELPA" packaging sorted out, but this might be some time. If
it would be helpful for the indentation tests, I could add this.

Phil

Re: moving some indentation tests

[Noam Postavsky]

On Sun, Apr 2, 2017 at 4:31 PM, Phillip Lord <phillip.lord@russet.org.uk> wrote:
>
> My own package, assess, provides good support for this kind of
> functionality. In particular, you can do this:
>
>
> (assess-roundtrip-indentation=
>  'emacs-lisp-mode
>  "
> (assess-with-find-file
>     \"~/.emacs\"
>   (buffer-string))")

For indent-sexp tests I just wrote this sort of thing in line (see
test/lisp/emacs-lisp/lisp-mode-tests.el). Would have been handy to
have some pre-written functions for it. After a quick glance I think
there are two issues that would get in the way of using the code
currently in assess for those tests though (ignoring the non-Emacs
deps).

I found that adding indentation and then reindenting turned up some
different bugs than removing indentation and reindenting. It looks
like assess only does the latter right now.

Also, AFAICT, it doesn't preserve indentation inside multi-line string literals.

Re: moving some indentation tests

[Phillip Lord]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> On Sun, Apr 2, 2017 at 4:31 PM, Phillip Lord <phillip.lord@russet.org.uk> wrote:
>>
>> My own package, assess, provides good support for this kind of
>> functionality. In particular, you can do this:
>>
>>
>> (assess-roundtrip-indentation=
>>  'emacs-lisp-mode
>>  "
>> (assess-with-find-file
>>     \"~/.emacs\"
>>   (buffer-string))")
>
> For indent-sexp tests I just wrote this sort of thing in line (see
> test/lisp/emacs-lisp/lisp-mode-tests.el). Would have been handy to
> have some pre-written functions for it. After a quick glance I think
> there are two issues that would get in the way of using the code
> currently in assess for those tests though (ignoring the non-Emacs
> deps).
>
> I found that adding indentation and then reindenting turned up some
> different bugs than removing indentation and reindenting. It looks
> like assess only does the latter right now.

Indeed it does. Do you have an example of the former? I am not sure
exactly the test proceedure that you are thinking of but I'd be happy to
add this. I think this kind of thing is why having pre-rolled
functions is good; getting this right is worth the effort.


> Also, AFAICT, it doesn't preserve indentation inside multi-line string
> literals.

It should do. This example:

(assess-roundtrip-indentation=
  'emacs-lisp-mode
"
(assess-with-find-file
    \"~/.emacs\"
  (buffer-string))")

returns "t" for me.

Phil

Re: moving some indentation tests

[Noam Postavsky]

On Mon, Apr 3, 2017 at 7:11 AM, Phillip Lord <phillip.lord@russet.org.uk> wrote:
>>
>> I found that adding indentation and then reindenting turned up some
>> different bugs than removing indentation and reindenting. It looks
>> like assess only does the latter right now.
>
> Indeed it does. Do you have an example of the former?

In (ert-deftest indent-sexp ()...) in
test/lisp/emacs-lisp/lisp-mode-tests.el where it says ";; Correctly
remove indentation". It's fairly simplistic, I just add an increasing
number of leading spaces to every line except for blank ones and lines
containing "noindent" (that's to avoid string literals). Oh, current
master is missing the check for blanks lines though, that's still
pending on my local branch:

              (unless (looking-at "noindent\\|^[[:blank:]]*$")
                (insert (make-string n ?\s)))

>
> (assess-roundtrip-indentation=
>   'emacs-lisp-mode
> "
> (assess-with-find-file
>     \"~/.emacs\"
>   (buffer-string))")
>
> returns "t" for me.

I meant multi-line string literals in the code being indented. I might
be misreading, but I think

(assess-roundtrip-indentation=
 'emacs-lisp-mode "\
\"a
\s\s\s\s\smulti
\s\sline
string-literal
\s\s\swith random leading space\"")

would return nil.

Re: moving some indentation tests

[Phillip Lord]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> On Mon, Apr 3, 2017 at 7:11 AM, Phillip Lord <phillip.lord@russet.org.uk> wrote:
>>>
>>> I found that adding indentation and then reindenting turned up some
>>> different bugs than removing indentation and reindenting. It looks
>>> like assess only does the latter right now.
>>
>> Indeed it does. Do you have an example of the former?
>
> In (ert-deftest indent-sexp ()...) in
> test/lisp/emacs-lisp/lisp-mode-tests.el where it says ";; Correctly
> remove indentation". It's fairly simplistic, I just add an increasing
> number of leading spaces to every line except for blank ones and lines
> containing "noindent" (that's to avoid string literals). Oh, current
> master is missing the check for blanks lines though, that's still
> pending on my local branch:
>
>               (unless (looking-at "noindent\\|^[[:blank:]]*$")
>                 (insert (make-string n ?\s)))

Ah, so you put in some relatively random indentation (well, not random),
and then reindent it, rather than starting from totally unindented.

I will add that, if it gives different results!

>>
>> (assess-roundtrip-indentation=
>>   'emacs-lisp-mode
>> "
>> (assess-with-find-file
>>     \"~/.emacs\"
>>   (buffer-string))")
>>
>> returns "t" for me.
>
> I meant multi-line string literals in the code being indented. I might
> be misreading, but I think
>
> (assess-roundtrip-indentation=
>  'emacs-lisp-mode "\
> \"a
> \s\s\s\s\smulti
> \s\sline
> string-literal
> \s\s\swith random leading space\"")
>
> would return nil.

Just so. The diff is:

Differ at:*** /tmp/a6466fOB	2017-04-03 17:32:04.268554341 +0100
--- /tmp/b6466sYH	2017-04-03 17:32:04.316554609 +0100
***************
*** 1,5 ****
  \"a
! multi
! line
  string-literal
! with random leading space\"
\\ No newline at end of file
--- 1,5 ----
  \"a
!      multi
!   line
  string-literal
!    with random leading space\"
\\ No newline at end of file

"

So the string gets indented.

Although I think emacs-lisp-mode is not working correctly here. In my hands:

"
(hello
(world))
"

indents to

"
(hello
 (world))
"

in emacs-lisp mode. So I'm not 100% sure this is assess not working.

Re: moving some indentation tests

[Noam Postavsky]

On Mon, Apr 3, 2017 at 12:39 PM, Phillip Lord
<phillip.lord@russet.org.uk> wrote:
>
> Ah, so you put in some relatively random indentation (well, not random),
> and then reindent it, rather than starting from totally unindented.

Yes, exactly. Arbitrary, but not random because if you have a test
failure it would be annoying to debug it with different indentation
each time!

>
> Although I think emacs-lisp-mode is not working correctly here. In my hands:
>
> "
> (hello
> (world))
> "
>
> indents to
>
> "
> (hello
>  (world))
> "
>
> in emacs-lisp mode. So I'm not 100% sure this is assess not working.

Ah, because lisp-indent-line doesn't check the parse context.
`indent-sexp' does the right thing, if called outside the string (and
also inside since #21343 was fixed).

I believe my patches at
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=25122#77 will fix this
for `indent-region' (since I happened to copy the syntax-ppss fix from
#21343). Probably `lisp-indent-line' should also use syntax-ppss to
get the correct parse context.

Re: moving some indentation tests

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> I'm thinking I would move the files to test/data and then write a new
Tom> test that simply iterates over these files and checking that
Tom> re-indentation works.

This turns out to be kind of a pain because there's no good way to pick
a name for the new file.  I tried test/lisp/indentation-tests.el, but
then the Makefile wants to find a dependency lisp/indentation.el.  I
wanted to choose a name that *wasn't* the same as any other existing
lisp file, because this test crosses file boundaries.

Any advice?  It seems to me that this indentation scheme is not so
great, in that it forces all the tests to appear in a single file, which
could be unwieldy for larger lisp sources.

Tom

Re: moving some indentation tests

[Eli Zaretskii]

> From: Tom Tromey <tom@tromey.com>
> Date: Wed, 05 Apr 2017 21:28:05 -0600
> Cc: Emacs discussions <emacs-devel@gnu.org>
> 
> >>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
> 
> Tom> I'm thinking I would move the files to test/data and then write a new
> Tom> test that simply iterates over these files and checking that
> Tom> re-indentation works.
> 
> This turns out to be kind of a pain because there's no good way to pick
> a name for the new file.  I tried test/lisp/indentation-tests.el, but
> then the Makefile wants to find a dependency lisp/indentation.el.  I
> wanted to choose a name that *wasn't* the same as any other existing
> lisp file, because this test crosses file boundaries.
> 
> Any advice?

How about picking up a name of an existing source file that is somehow
related to the tests?

If that's not good enough, I think we need some new infrastructure for
this in test/Makefile, e.g. a way of specifying a rule that tells Make
which source files are prerequisites for the given test file, so that
when one of the prerequisites changes, the tests are automatically
run.

Re: moving some indentation tests

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

> If that's not good enough, I think we need some new infrastructure for
> this in test/Makefile, e.g. a way of specifying a rule that tells Make
> which source files are prerequisites for the given test file, so that
> when one of the prerequisites changes, the tests are automatically
> run.

That would be great! tramp-tests.el depends on much more files but
tramp.el.

Best regards, Michael.

Re: moving some indentation tests

[Noam Postavsky]

On Thu, Apr 6, 2017 at 10:11 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Tom Tromey <tom@tromey.com>
>> Date: Wed, 05 Apr 2017 21:28:05 -0600
>> Cc: Emacs discussions <emacs-devel@gnu.org>
>>
>> >>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
>>
>> Tom> I'm thinking I would move the files to test/data and then write a new
>> Tom> test that simply iterates over these files and checking that
>> Tom> re-indentation works.
>>
>> This turns out to be kind of a pain because there's no good way to pick
>> a name for the new file.  I tried test/lisp/indentation-tests.el, but
>> then the Makefile wants to find a dependency lisp/indentation.el.  I
>> wanted to choose a name that *wasn't* the same as any other existing
>> lisp file, because this test crosses file boundaries.
>>
>> Any advice?
>
> How about picking up a name of an existing source file that is somehow
> related to the tests?

Yeah, shouldn't the js indentation tests go in lisp/progmodes/js-tests.el, etc?

Re: moving some indentation tests

[Tom Tromey]

>>>>> "Noam" == Noam Postavsky <npostavs@users.sourceforge.net> writes:

>> How about picking up a name of an existing source file that is somehow
>> related to the tests?

Noam> Yeah, shouldn't the js indentation tests go in lisp/progmodes/js-tests.el, etc?

The current tests are just a bunch of files that should not be changed
by re-indentation.  This has the nice property that a new test can be
written by just dropping a file in the correct directory.

It's possible of course to split these by major mode.  But, that's
significantly more work, both for moving the tests and for writing new
tests, and furthermore, IMO of course, doesn't provide any real benefit.

Tom

Re: moving some indentation tests

[Phillip Lord]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> On Mon, Apr 3, 2017 at 12:39 PM, Phillip Lord
> <phillip.lord@russet.org.uk> wrote:
>>
>> Ah, so you put in some relatively random indentation (well, not random),
>> and then reindent it, rather than starting from totally unindented.
>
> Yes, exactly. Arbitrary, but not random because if you have a test
> failure it would be annoying to debug it with different indentation
> each time!
>
>>
>> Although I think emacs-lisp-mode is not working correctly here. In my hands:
>>
>> "
>> (hello
>> (world))
>> "
>>
>> indents to
>>
>> "
>> (hello
>>  (world))
>> "
>>
>> in emacs-lisp mode. So I'm not 100% sure this is assess not working.
>
> Ah, because lisp-indent-line doesn't check the parse context.
> `indent-sexp' does the right thing, if called outside the string (and
> also inside since #21343 was fixed).
>
> I believe my patches at
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=25122#77 will fix this
> for `indent-region' (since I happened to copy the syntax-ppss fix from
> #21343). Probably `lisp-indent-line' should also use syntax-ppss to
> get the correct parse context.

So, in this case, assess is working? lisp-indent-line is, effectively,
broken?

Phil

Re: moving some indentation tests

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:
>> Any advice?
>
> How about picking up a name of an existing source file that is somehow
> related to the tests?
>
> If that's not good enough, I think we need some new infrastructure for
> this in test/Makefile, e.g. a way of specifying a rule that tells Make
> which source files are prerequisites for the given test file, so that
> when one of the prerequisites changes, the tests are automatically
> run.

The current scheme just uses a heuristic to guess the name of the
prerequisities. Where this doesn't work, there is nothing to prevent
people from adding these by hand, of course. That would be a good thing
in my book. If there is enough commonality, we can add new heuristics to
generate the manual rules.

Phil

Re: moving some indentation tests

[Noam Postavsky]

On Fri, Apr 7, 2017 at 11:51 AM, Phillip Lord
<phillip.lord@russet.org.uk> wrote:
> Noam Postavsky <npostavs@users.sourceforge.net> writes:
>> Ah, because lisp-indent-line doesn't check the parse context.
>> `indent-sexp' does the right thing, if called outside the string (and
>> also inside since #21343 was fixed).
>>
>> I believe my patches at
>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=25122#77 will fix this
>> for `indent-region' (since I happened to copy the syntax-ppss fix from
>> #21343). Probably `lisp-indent-line' should also use syntax-ppss to
>> get the correct parse context.
>
> So, in this case, assess is working? lisp-indent-line is, effectively,
> broken?

I think both assess and lisp-indent-line are broken for this case, in
different ways. js-indent-line seems to handle this correctly though.
So you should be able to test assess with js-mode.