[Bug] commit 39070b7fc7 breaks babel test

[Bug] commit 39070b7fc7 breaks babel test

[Achim Gratz]

Hi Eric,

this change seems to introduce additional line breaks in the following
test:

--8<---------------cut here---------------start------------->8---
Test test-ob/catches-all-references condition:
    (ert-test-failed
     ((should
       (string=
        (org-babel-execute-src-block)
        "A literal example\non two lines for me."))
      :form
      (string= "A literal example\non two lines\n for me." "A literal example\non two lines for me.")
      :value nil))
   FAILED  1/1  test-ob/catches-all-references
--8<---------------cut here---------------end--------------->8---

This seems to happen because the final \n from the babel result is not
stripped anymore, pointing to the change in ob-core.  IIRC we
flip-flopped a few times already with including or not including this
final newline, so I don't know whether the code or the test should
change.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: [Bug] commit 39070b7fc7 breaks babel test

[Skip Collins]

It's been a week and this test still fails.

Would it make sense to automatically enforce passing all tests before
git accepts a change?

On Wed, Nov 27, 2013 at 2:58 AM, Achim Gratz <Stromeko@nexgo.de> wrote:
>
> Hi Eric,
>
> this change seems to introduce additional line breaks in the following
> test:
>
> --8<---------------cut here---------------start------------->8---
> Test test-ob/catches-all-references condition:
>     (ert-test-failed
>      ((should
>        (string=
>         (org-babel-execute-src-block)
>         "A literal example\non two lines for me."))
>       :form
>       (string= "A literal example\non two lines\n for me." "A literal example\non two lines for me.")
>       :value nil))
>    FAILED  1/1  test-ob/catches-all-references
> --8<---------------cut here---------------end--------------->8---
>
> This seems to happen because the final \n from the babel result is not
> stripped anymore, pointing to the change in ob-core.  IIRC we
> flip-flopped a few times already with including or not including this
> final newline, so I don't know whether the code or the test should
> change.

Re: [Bug] commit 39070b7fc7 breaks babel test

[Eric Schulte]

Skip Collins <skip.collins@gmail.com> writes:

> It's been a week and this test still fails.
>

Fixed.

>
> Would it make sense to automatically enforce passing all tests before
> git accepts a change?
>

I for one would strongly oppose this change.  This would only make it
take longer and thus make it less likely that new code is committed.
This is the master branch where development should be fast and
experimentation should take place, not the maintenance branch.

Best,

>
> On Wed, Nov 27, 2013 at 2:58 AM, Achim Gratz <Stromeko@nexgo.de> wrote:
>>
>> Hi Eric,
>>
>> this change seems to introduce additional line breaks in the following
>> test:
>>
>> --8<---------------cut here---------------start------------->8---
>> Test test-ob/catches-all-references condition:
>>     (ert-test-failed
>>      ((should
>>        (string=
>>         (org-babel-execute-src-block)
>>         "A literal example\non two lines for me."))
>>       :form
>>       (string= "A literal example\non two lines\n for me." "A literal example\non two lines for me.")
>>       :value nil))
>>    FAILED  1/1  test-ob/catches-all-references
>> --8<---------------cut here---------------end--------------->8---
>>
>> This seems to happen because the final \n from the babel result is not
>> stripped anymore, pointing to the change in ob-core.  IIRC we
>> flip-flopped a few times already with including or not including this
>> final newline, so I don't know whether the code or the test should
>> change.
>

-- 
Eric Schulte
https://cs.unm.edu/~eschulte
PGP: 0x614CA05D

Re: [Bug] commit 39070b7fc7 breaks babel test

[Achim Gratz]

Eric Schulte writes:
> Fixed.

The test was trying to ascertain that an inline babel codeblock didn't
force a newline at the end.  You've just made it test that there is a
trailing newline introduced by the inline babel call, which is clearly
against its intent.  You could have marked it as a known fail if you
were planning to have this as a transitory failure only.  I can't find
the discussion that led to the former implementation at the moment, but
I'm fairly certain that there was a bug report against trailing newlines
from inline babel calls.  So what is the rationale of re-introducing the
trailing newline this time around?


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Samples for the Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#BlofeldSamplesExtra

Re: [Bug] commit 39070b7fc7 breaks babel test

[Eric Schulte]

Achim Gratz <Stromeko@nexgo.de> writes:

> Eric Schulte writes:
>> Fixed.
>
> The test was trying to ascertain that an inline babel codeblock didn't
> force a newline at the end.

What makes this code block inline?

> You've just made it test that there is a trailing newline introduced
> by the inline babel call, which is clearly against its intent.  You
> could have marked it as a known fail if you were planning to have this
> as a transitory failure only.  I can't find the discussion that led to
> the former implementation at the moment, but I'm fairly certain that
> there was a bug report against trailing newlines from inline babel
> calls.  So what is the rationale of re-introducing the trailing
> newline this time around?
>

This test (test-ob/catches-all-references) is from commit c21692506d8,
which doesn't have anything to do with newlines (judging from the commit
message).

To me the more natural behavior is to include the newline in the
expansion.  Maybe we have discussed this before on list, and decided
stripping the newline was preferable, but I don't recall that
discussion.

Just because behavior ends up being encoded in a test doesn't
necessarily mean the behavior is correct.  I think as test suites
attempt to protect the desired behavior they often end up also
protecting incidental behavior of the implementation at the time the
test was written.

Best,

>
>
> Regards,
> Achim.

-- 
Eric Schulte
https://cs.unm.edu/~eschulte
PGP: 0x614CA05D

Re: [Bug] commit 39070b7fc7 breaks babel test

[Achim Gratz]

Eric Schulte writes:
> This test (test-ob/catches-all-references) is from commit c21692506d8,
> which doesn't have anything to do with newlines (judging from the commit
> message).
>
> To me the more natural behavior is to include the newline in the
> expansion.  Maybe we have discussed this before on list, and decided
> stripping the newline was preferable, but I don't recall that
> discussion.

I seem to have misremembered.  Anyway, when you committed that test you
apparently thought it was more natural to leave the trailing newline
out.  I'm not using such constructs myself, but changing this behaviour
more than two years later carries a risk that people have documents that
rely on it, for good or bad reasons.

> Just because behavior ends up being encoded in a test doesn't
> necessarily mean the behavior is correct.  I think as test suites
> attempt to protect the desired behavior they often end up also
> protecting incidental behavior of the implementation at the time the
> test was written.

Well, a test test should proof the specification is implemented
correctly.  This is exactly why test methodology has evolved to unit
tests, which mostly avoids testing for incident behaviour or emergent
phenomena.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Wavetables for the Terratec KOMPLEXER:
http://Synth.Stromeko.net/Downloads.html#KomplexerWaves

Re: [Bug] commit 39070b7fc7 breaks babel test

[Skip Collins]

On Fri, Dec 6, 2013 at 2:05 PM, Eric Schulte <schulte.eric@gmail.com> wrote:
> Skip Collins <skip.collins@gmail.com> writes:
>> Would it make sense to automatically enforce passing all tests before
>> git accepts a change?
>
> I for one would strongly oppose this change.  This would only make it
> take longer and thus make it less likely that new code is committed.
> This is the master branch where development should be fast and
> experimentation should take place, not the maintenance branch.

Designating something as an expected failure seems to be a good way to
track minor issues that need eventually to be resolved. As a user, I
frequently update with make up2 just to avoid getting bitten by stupid
errors that might sneak into master. Is it really that much extra work
for a developer to run the same command before committing and either
fix the error or mark it as a known failure?

Re: [Bug] commit 39070b7fc7 breaks babel test

[Bastien]

Hi Skip,

Skip Collins <skip.collins@gmail.com> writes:

> Designating something as an expected failure seems to be a good way to
> track minor issues that need eventually to be resolved. As a user, I
> frequently update with make up2 just to avoid getting bitten by stupid
> errors that might sneak into master. Is it really that much extra work
> for a developer to run the same command before committing and either
> fix the error or mark it as a known failure?

I'm for a relaxed policy here.

Users living on the master branch know they are testing an unstable
version of Org, and such hiccups just happen.

-- 
 Bastien

Re: [Bug] commit 39070b7fc7 breaks babel test

[Eric Schulte]

Skip Collins <skip.collins@gmail.com> writes:

> On Fri, Dec 6, 2013 at 2:05 PM, Eric Schulte <schulte.eric@gmail.com> wrote:
>> Skip Collins <skip.collins@gmail.com> writes:
>>> Would it make sense to automatically enforce passing all tests before
>>> git accepts a change?
>>
>> I for one would strongly oppose this change.  This would only make it
>> take longer and thus make it less likely that new code is committed.
>> This is the master branch where development should be fast and
>> experimentation should take place, not the maintenance branch.
>
> Designating something as an expected failure seems to be a good way to
> track minor issues that need eventually to be resolved. As a user, I
> frequently update with make up2 just to avoid getting bitten by stupid
> errors that might sneak into master. Is it really that much extra work
> for a developer to run the same command before committing and either
> fix the error or mark it as a known failure?

If it increases the time taken to make a change by say 25%, then it will
result in me addressing only 4/5 as many issues.  I personally favor

1. a flexible master branch where we can try things out and spur
   discussion

2. a setup with less hurdles to committing---it's easy to revert a
   commit, but impossible recover a commit which is never made

Best,

-- 
Eric Schulte
https://cs.unm.edu/~eschulte
PGP: 0x614CA05D