emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
* [Bug] commit 39070b7fc7 breaks babel test
@ 2013-11-27  7:58 Achim Gratz
  2013-12-03 19:09 ` Skip Collins
  0 siblings, 1 reply; 9+ messages in thread
From: Achim Gratz @ 2013-11-27  7:58 UTC (permalink / raw)
  To: emacs-orgmode

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Bug] commit 39070b7fc7 breaks babel test
  2013-11-27  7:58 [Bug] commit 39070b7fc7 breaks babel test Achim Gratz
@ 2013-12-03 19:09 ` Skip Collins
  2013-12-06 19:05   ` Eric Schulte
  0 siblings, 1 reply; 9+ messages in thread
From: Skip Collins @ 2013-12-03 19:09 UTC (permalink / raw)
  To: Achim Gratz; +Cc: emacs-org list

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Bug] commit 39070b7fc7 breaks babel test
  2013-12-03 19:09 ` Skip Collins
@ 2013-12-06 19:05   ` Eric Schulte
  2013-12-06 19:47     ` Achim Gratz
  2013-12-06 20:51     ` Skip Collins
  0 siblings, 2 replies; 9+ messages in thread
From: Eric Schulte @ 2013-12-06 19:05 UTC (permalink / raw)
  To: Skip Collins; +Cc: Achim Gratz, emacs-org list

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Bug] commit 39070b7fc7 breaks babel test
  2013-12-06 19:05   ` Eric Schulte
@ 2013-12-06 19:47     ` Achim Gratz
  2013-12-07  2:27       ` Eric Schulte
  2013-12-06 20:51     ` Skip Collins
  1 sibling, 1 reply; 9+ messages in thread
From: Achim Gratz @ 2013-12-06 19:47 UTC (permalink / raw)
  To: emacs-orgmode

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Bug] commit 39070b7fc7 breaks babel test
  2013-12-06 19:05   ` Eric Schulte
  2013-12-06 19:47     ` Achim Gratz
@ 2013-12-06 20:51     ` Skip Collins
  2013-12-06 21:48       ` Bastien
  2013-12-07  2:31       ` Eric Schulte
  1 sibling, 2 replies; 9+ messages in thread
From: Skip Collins @ 2013-12-06 20:51 UTC (permalink / raw)
  To: Eric Schulte; +Cc: Achim Gratz, emacs-org list

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?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Bug] commit 39070b7fc7 breaks babel test
  2013-12-06 20:51     ` Skip Collins
@ 2013-12-06 21:48       ` Bastien
  2013-12-07  2:31       ` Eric Schulte
  1 sibling, 0 replies; 9+ messages in thread
From: Bastien @ 2013-12-06 21:48 UTC (permalink / raw)
  To: Skip Collins; +Cc: Achim Gratz, emacs-org list, Eric Schulte

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Bug] commit 39070b7fc7 breaks babel test
  2013-12-06 19:47     ` Achim Gratz
@ 2013-12-07  2:27       ` Eric Schulte
  2013-12-07  8:56         ` Achim Gratz
  0 siblings, 1 reply; 9+ messages in thread
From: Eric Schulte @ 2013-12-07  2:27 UTC (permalink / raw)
  To: Achim Gratz; +Cc: emacs-orgmode

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Bug] commit 39070b7fc7 breaks babel test
  2013-12-06 20:51     ` Skip Collins
  2013-12-06 21:48       ` Bastien
@ 2013-12-07  2:31       ` Eric Schulte
  1 sibling, 0 replies; 9+ messages in thread
From: Eric Schulte @ 2013-12-07  2:31 UTC (permalink / raw)
  To: Skip Collins; +Cc: Achim Gratz, emacs-org list

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Bug] commit 39070b7fc7 breaks babel test
  2013-12-07  2:27       ` Eric Schulte
@ 2013-12-07  8:56         ` Achim Gratz
  0 siblings, 0 replies; 9+ messages in thread
From: Achim Gratz @ 2013-12-07  8:56 UTC (permalink / raw)
  To: emacs-orgmode

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2013-12-07  8:56 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-27  7:58 [Bug] commit 39070b7fc7 breaks babel test Achim Gratz
2013-12-03 19:09 ` Skip Collins
2013-12-06 19:05   ` Eric Schulte
2013-12-06 19:47     ` Achim Gratz
2013-12-07  2:27       ` Eric Schulte
2013-12-07  8:56         ` Achim Gratz
2013-12-06 20:51     ` Skip Collins
2013-12-06 21:48       ` Bastien
2013-12-07  2:31       ` Eric Schulte

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).