Regtest changes phase 1

Fingers crossed for this one...

[PhilEHolmes, 2012-08-08 15:49:48]

Please review initial regtest updates

[Graham Percival, 2012-08-08 16:09:57]

initial review of initial regtest changes.

http://codereview.appspot.com/6454121/diff/1/input/regression/context-mod-wit...
File input/regression/context-mod-with.ly (right):

http://codereview.appspot.com/6454121/diff/1/input/regression/context-mod-wit...
input/regression/context-mod-with.ly:4: texidoc = "Context modifications can be
stored into a variable as a
this file looks much more complicated and should be reviewed by somebody who
knows about context modifications (i.e. not me)

http://codereview.appspot.com/6454121/diff/1/input/regression/markup-user.ly
File input/regression/markup-user.ly (right):

http://codereview.appspot.com/6454121/diff/1/input/regression/markup-user.ly#...
input/regression/markup-user.ly:16: c''-\markup \upcase #"hello world in upper
case"
This file can be pushed directly to staging.

http://codereview.appspot.com/6454121/diff/1/input/regression/relative-repeat.ly
File input/regression/relative-repeat.ly (right):

http://codereview.appspot.com/6454121/diff/1/input/regression/relative-repeat...
input/regression/relative-repeat.ly:10: \alternative { a1_"Alt1" e_"Alt2"
b_"Alt3" }
Isn't \alternative support to have the format
  { {first} {second} {third} }
?  I know that we can drop the inner {} if it's only a single note, but IMO that
isn't as safe as adding the {} and there's no harm in having the inner {}
explicitly.

Why is the \clef bass important?  Perhaps that should be mentioned in the
texidoc?

Why remove the \relative c' ?

[PhilEHolmes, 2012-08-08 16:32:38]

http://codereview.appspot.com/6454121/diff/1/input/regression/context-mod-wit...
File input/regression/context-mod-with.ly (right):

http://codereview.appspot.com/6454121/diff/1/input/regression/context-mod-wit...
input/regression/context-mod-with.ly:4: texidoc = "Context modifications can be
stored into a variable as a
The context mods have not been touched.  All I've done is documented what they
look like in the regtest, using /mark to do the documentation.  If you're happy
with that, this should be good to go.

http://codereview.appspot.com/6454121/diff/1/input/regression/relative-repeat.ly
File input/regression/relative-repeat.ly (right):

http://codereview.appspot.com/6454121/diff/1/input/regression/relative-repeat...
input/regression/relative-repeat.ly:10: \alternative { a1_"Alt1" e_"Alt2"
b_"Alt3" }
I used the bass clef because without \relative c' it looked daft.  I took the
\relative out because it was originally documented as "Relative mode for
repeats" so I thought the \relative was confusing things.  Happy to restore it
and get rid of the bass clef, though.  Also happy to add the {} syntax on the
alternatives and change the text.

[Trevor Daniels, 2012-08-08 18:15:44]

Sorry Phil, but I don't think this is an improvement.

a) The original code comments explain much more clearly
what is being tested than do your new marks.

b) Although the marks don't affect the test they do
tend to mask the real tests and make it harder to see
what is really being tested.

I'd leave the code alone and expand the description if
further clarity is needed.

Trevor

[Trevor Daniels, 2012-08-08 18:17:20]

My earlier comments apply only to the first of these three
regtests.  I haven't looked at the others yet.

[Trevor Daniels, 2012-08-08 18:23:38]

http://codereview.appspot.com/6454121/diff/1/input/regression/relative-repeat.ly
File input/regression/relative-repeat.ly (right):

http://codereview.appspot.com/6454121/diff/1/input/regression/relative-repeat...
input/regression/relative-repeat.ly:10: \alternative { a1_"Alt1" e_"Alt2"
b_"Alt3" }
I haven't checked, but I suspect the origin of this
regtest was because there was a problem with \alternative
within a \relative block, given the texidoc string.  In
which case the \relative is a pertinent part of the test.
I suggest expanding the test to include abolute and
relative modes.

Trevor

[email_philholmes.net, 2012-08-08 18:41:43]

----- Original Message ----- 
From: <tdanielsmusic@googlemail.com>
To: <PhilEHolmes@googlemail.com>; <graham@percival-music.ca>
Cc: <lilypond-devel@gnu.org>; <reply@codereview-hr.appspotmail.com>
Sent: Wednesday, August 08, 2012 7:15 PM
Subject: Re: Regtest changes phase 1 (issue 6454121)


> Sorry Phil, but I don't think this is an improvement.
>
> a) The original code comments explain much more clearly
> what is being tested than do your new marks.
>
> b) Although the marks don't affect the test they do
> tend to mask the real tests and make it harder to see
> what is really being tested.
>
> I'd leave the code alone and expand the description if
> further clarity is needed.
>
> Trevor


Absolutely appreciate this comment.  However, the theory of the regtests is 
that by looking at the description and the image, you can tell whether the 
regtest has been passed.  No-one who looked at this regtest could do that. 
With the changes proposed, you can.   The question is - do you need to 
inspect the code, or the image and description, to tell whether the regtest 
has been passed?

--
Phil Holmes 

[mail_philholmes.net, 2012-08-08 18:42:44]

----- Original Message ----- 
From: <tdanielsmusic@googlemail.com>
To: <PhilEHolmes@googlemail.com>; <graham@percival-music.ca>
Cc: <reply@codereview-hr.appspotmail.com>; <lilypond-devel@gnu.org>
Sent: Wednesday, August 08, 2012 7:23 PM
Subject: Re: Regtest changes phase 1 (issue 6454121)


>
>
http://codereview.appspot.com/6454121/diff/1/input/regression/relative-repeat.ly
> File input/regression/relative-repeat.ly (right):
>
>
http://codereview.appspot.com/6454121/diff/1/input/regression/relative-repeat...
> input/regression/relative-repeat.ly:10: \alternative { a1_"Alt1"
> e_"Alt2" b_"Alt3" }
> I haven't checked, but I suspect the origin of this
> regtest was because there was a problem with \alternative
> within a \relative block, given the texidoc string.  In
> which case the \relative is a pertinent part of the test.
> I suggest expanding the test to include abolute and
> relative modes.
>
> Trevor
>
> http://codereview.appspot.com/6454121/

I did try, and I could see no effect from \relative.  It seemed to me to be 
a red herring.

--
Phil Holmes 

[Trevor Daniels, 2012-08-08 19:02:13]

On 2012/08/08 18:42:44, mail_philholmes.net wrote:

> I did try, and I could see no effect from \relative.

Of course.  It's working correctly now.  The point of
a regtest is to ensure it continues to work correctly.

[Trevor Daniels, 2012-08-08 19:05:26]

On 2012/08/08 18:41:43, email_philholmes.net wrote:

> However, the theory of the regtests is that by looking 
> at the description and the image, you can tell whether the 
> regtest has been passed.

Yes.  To improve that you can change either the image or
the description.  In this case I think changing the
description would be better.

[email_philholmes.net, 2012-08-10 13:55:51]

----- Original Message ----- 
From: <tdanielsmusic@googlemail.com>
To: <PhilEHolmes@googlemail.com>; <graham@percival-music.ca>
Cc: <lilypond-devel@gnu.org>; <reply@codereview-hr.appspotmail.com>
Sent: Wednesday, August 08, 2012 7:15 PM
Subject: Re: Regtest changes phase 1 (issue 6454121)


> Sorry Phil, but I don't think this is an improvement.
>
> a) The original code comments explain much more clearly
> what is being tested than do your new marks.

But when you're checking the regtests, you don't see the code at all.  To 
make visual checking possible, there has to be a description of what you're 
looking at, not what the test does.  See something like 
autobeam-show-defaults.ly as a good example of this approach.  I've tried to 
follow this principle in the change I've suggested.

> b) Although the marks don't affect the test they do
> tend to mask the real tests and make it harder to see
> what is really being tested.
>
> I'd leave the code alone and expand the description if
> further clarity is needed.

When there's as many staves as there are with the test we're discussing, 
that would make the description very long and impossible to follow: "and the 
seventh stave has an ambitus added and a clef taken away" or somesuch.  Have 
a look at the collated-files for the current context-mod-with regtest, and 
compare it with the result from processing my proposed change, and ask 
yourself "Can I tell if either of these ran without error?"


AFAIK, the purpose of the regtests isn't to show clear code - it's to 
deliver code that tests a specific aspect of functionality, and where 
inspection of the output shows that functionality is working.

--
Phil Holmes 

[PhilEHolmes, 2012-08-10 14:05:38]

Please review updated patch set.

[Graham Percival, 2012-08-10 14:12:13]

http://codereview.appspot.com/6454121/diff/11001/input/regression/relative-re...
File input/regression/relative-repeat.ly (right):

http://codereview.appspot.com/6454121/diff/11001/input/regression/relative-re...
input/regression/relative-repeat.ly:10: \alternative { { a2_"Alt1" c } {
e_"Alt2" c } { b_"Alt3" d } }
This file LGTM.

[Trevor Daniels, 2012-08-13 18:48:13]

http://codereview.appspot.com/6454121/diff/11001/input/regression/markup-user.ly
File input/regression/markup-user.ly (right):

http://codereview.appspot.com/6454121/diff/11001/input/regression/markup-user...
input/regression/markup-user.ly:22: \override PaperColumn #'keep-inside-line =
##f
This file LGTM.

Trevor

http://codereview.appspot.com/6454121/diff/11001/input/regression/relative-re...
File input/regression/relative-repeat.ly (right):

http://codereview.appspot.com/6454121/diff/11001/input/regression/relative-re...
input/regression/relative-repeat.ly:10: \alternative { { a2_"Alt1" c } {
e_"Alt2" c } { b_"Alt3" d } }
On 2012/08/10 14:12:13, Graham Percival wrote:

> This file LGTM.

... and to me.

[PhilEHolmes, 2012-08-14 09:54:37]

Please review final mods.

[Keith, 2012-08-15 05:10:13]

Sorry I didn't notice this before.

'relative-repeat.ly' was testing something different than you thought.

http://codereview.appspot.com/6454121/diff/12002/input/regression/relative-re...
File input/regression/relative-repeat.ly (left):

http://codereview.appspot.com/6454121/diff/12002/input/regression/relative-re...
input/regression/relative-repeat.ly:2: texidoc = "Relative mode for repeats uses
order of entry."
The point of the original regression test was that
  \relative c' {
   \repeat unfold 3 {f2 bes2}
   \alternative { {a1}{e}{b} } }
is very different from a textual unfolding of the repeats.
  \relative c' {f2 bes2 a1 f2 bes2 e f2 bes2 b }

Relative-pitch-entry, and default-durations follow the order of typed input, not
the order of printed output.

Barchecks and octave checks would make the test self-explanatory.  Maybe there
are enough other uses of relative mode with repeats that we could leave this one
out.

http://codereview.appspot.com/6454121/diff/12002/input/regression/relative-re...
File input/regression/relative-repeat.ly (right):

http://codereview.appspot.com/6454121/diff/12002/input/regression/relative-re...
input/regression/relative-repeat.ly:2: texidoc = "Unfolded repeats take
alternative notes in the order
We don't need the replacement test, because 'repeat-unfold.ly' covers that
function.

[PhilEHolmes, 2012-08-18 13:42:52]

Please review

[Trevor Daniels, 2012-08-18 22:10:16]

On 2012/08/10 13:55:51, email_philholmes.net wrote:

> But when you're checking the regtests, you don't see the 
> code at all.  To  make visual checking possible, there has 
> to be a description of what you're looking at, not what the 
> test does.

OK, if we accept this, someone needs to look at the code
if a test changes to get a clue about where to begin looking.
Why remove the original comments?  They're even more important
now the code is so cluttered.  (This applies to just the
first test.)

[Graham Percival, 2012-08-20 18:38:13]

LGTM

http://codereview.appspot.com/6454121/diff/7/input/regression/relative-repeat.ly
File input/regression/relative-repeat.ly (right):

http://codereview.appspot.com/6454121/diff/7/input/regression/relative-repeat...
input/regression/relative-repeat.ly:4: system has alll the notes within the
stave.  In the second, the
typo: alll

[Trevor Daniels, 2012-08-21 14:43:43]

All LGTM now.  Thanks Phil!

Trevor