Issue 5318: Context regression tests

https://sourceforge.net/p/testlilyissues/issues/5318/

Note that context-find-parent.ly currently fails.

[Dan Eble, 2018-05-03 02:48:10]

I would appreciate a close review of these tests by at least one of the
long-time contributors or pro users.  Contexts are a central part of LilyPond
and if I've misjudged how any of these cases should work, I don't want it to
slip by.  Thanks.

If you have suggestions on a better approach to testing, I'm all ears.

James, please leave this in review until there is feedback.

[Carl, 2018-05-03 12:40:17]

On 2018/05/03 02:48:10, Dan Eble wrote:
> I would appreciate a close review of these tests by at least one of the
> long-time contributors or pro users.  Contexts are a central part of LilyPond
> and if I've misjudged how any of these cases should work, I don't want it to
> slip by.  Thanks.
> 
> If you have suggestions on a better approach to testing, I'm all ears.
> 
> James, please leave this in review until there is feedback.

This is a very impressive test suite.  You have done a great job of considering
possibilities and developing code to demonstrate them.

I'm a little bit surprised by the nature of this testing, however.  I have
always assumed that context id's should be unique, and that if one created two
contexts of the same type with the same id, the results would be undefined. 
You've looked at how the code works and ferreted out the behaviors you are
testing in this set of tests, but I don't believe that the behavior you have
identified should be contracturally-defined behavior.

In my opinion, the most ideal result when one tries to create a new context with
the same type and id of an existing context would be to generate an error,
something like "Error: duplicate Staff with ID of A".

If we promise the behavior that your regression tests demonstrate, then we have
developed an official scope for context IDs, and most LilyPond constructs do not
have scope.

But this is just my initial opinion, and I am certainly open to other arguments.

Thanks,

Carl

[Dan Eble, 2018-05-04 16:48:36]

On 2018/05/03 12:40:17, Carl wrote:
> In my opinion, the most ideal result when one tries to create a new context
with
> the same type and id of an existing context would be to generate an error,
> something like "Error: duplicate Staff with ID of A".

First, thanks for taking a look.

It seems impractical to eliminate ambiguity entirely, and probably pretty
difficult even to reduce it.

For one thing, context IDs are optional.  I guess you wouldn't want to start
forbidding the creation of multiple staves without any ID, but there is a choice
to make.

There are features (\partcombine comes to mind, and << \\ >> IIRC) that create
contexts with specific IDs that are not necessarily unique in the scope of a
score.  They can produce trees like context-find-grandchild-ambiguous.ly.  It
would be nice to emit a warning in that case.  I wonder about the impact of an
exhaustive search on performance.

It might be interesting to experiment with forbidding creation of
  * a context with the same type+ID as any of its ancestors
  * siblings of the same type+ID
and see what happens to the regression tests.  That might be worth something,
but unless the \change command were similarly limited, it might still be
possible to create weird trees by relocating branches.

(Note that when I say "it would be nice," I'm not volunteering to do these
things now.)

> If we promise the behavior that your regression tests demonstrate, then we
have
> developed an official scope for context IDs, and most LilyPond constructs do
not
> have scope.

Could I address your concern with comments in the test descriptions?  Which
ones?  Even if we consider some of this behavior subject to change, it's still
useful to be able to detect changes.

[Dan Eble, 2018-05-07 22:53:29]

stop relying on duplicating type+ID

[Dan Eble, 2018-05-07 22:56:59]

remove stray character from comment

[Dan Eble, 2018-05-07 23:07:36]

On 2018/05/07 22:53:29, Dan Eble wrote:
> stop relying on duplicating type+ID

Carl,

I hope that these revisions address your concerns about the tests per se.

[Carl, 2018-05-12 20:46:54]

On 2018/05/07 23:07:36, Dan Eble wrote:
> On 2018/05/07 22:53:29, Dan Eble wrote:
> > stop relying on duplicating type+ID
> 
> Carl,
> 
> I hope that these revisions address your concerns about the tests per se.

After reviewing the revised tests, I am in favor of moving back to your original
patch, with the exception of keeping the added recommendation about issuing a
warning if there are two potential matches for descendants of a particular
context.

I had not realized that the search for the known ID contexts follows exactly the
same behavior as the search for contexts without given IDs.

I think that having the context ID's helps to make the behavior clearer.

Maybe instead of using "FAIL" and "PASS" for the instrument names, it would be
better to use "ORIGINAL" when the name is first given, and then "PARENT",
"CHILD", "GRANDCHILD", "SIBLING" etc. to identify the context that is actually
found.  Just a thought that might help make the whole suite of tests easier to
understand.

Thanks for your work on this.

Carl

[Dan Eble, 2018-05-13 02:08:26]

restore IDs

[Dan Eble, 2018-05-13 02:10:16]

revise instrument names for clarity

[Dan Eble, 2018-05-13 18:07:15]

I no longer plan to push context-global-*.ly, though I won't refresh the code
review for it.

By the time I'm done with some changes I'm working on, I think I'll have to
overhaul those cases to take a different approach.