2015-05-03 9:06 GMT+03:00 Olaf Dabrunz <o...@fctrace.org>:
> On 02-May-15, Nikolay Pavlov wrote:
>> 2015-05-02 5:44 GMT+03:00 Olaf Dabrunz <o...@fctrace.org>:
>> > On 30-Apr-15, Nikolay Pavlov wrote:
>> >> 2015-04-30 10:56 GMT+03:00 Olaf Dabrunz <o...@fctrace.org>:
>> >> > diff -r 18d84ed365a5 src/testdir/test55.in
>> >> > --- a/src/testdir/test55.in Wed Apr 22 22:18:22 2015 +0200
>> >> > +++ b/src/testdir/test55.in Thu Apr 30 09:09:37 2015 +0200
>> >> > @@ -408,6 +408,20 @@ let l = [0, 1, 2, 3]
>> >> > :endtry
>> >> > :$put =string(d)
>> >> > :"
>> >> > +:$put ='extend() after lock on dict (existing: yes, new: no):'
>> >> > +:unlet! d
>> >> > +:let d = {'a': 99, 'b': 100}
>> >> > +:lockvar 1 d
>> >> > +:try
>> >> > +: $put =string(extend(d, {'a': 123}))
>> >> > +: $put ='did extend() existing item'
>> >>
>> >> I think :try should be here, not above.
>> >
>> > The :try block fails on the first extend() without the patch, and on the
>> > second one (as intended) with the patch. This tests for regressions.
>> >
>> > What would be achieved by moving the :try here?
>>
>> With `:try` block moved it will run both extend()s always. Test will
>> fail in any case: without move because of absense of two put lines and
>> also different dictionary value, with move because of wrong put output
>> when stringifying dictionary later and put=string(extend(...)) output
>> (with the error, but without :try it will put zero). By moving try you
>> show *where you expect* the exception.
>
> Ah, this is also a pattern to mark the expected error for the reader.
>
> (
> In the meantime, the next iterations of this test marked the
> expected passing test with 'ok:' and the expected error with
> 'wrong:' in the messages following each of them.
>
> But my marks only work well when the 'ok' tests precede the 'wrong'
> tests, and all are executed in a :try block. Yet, in a list of
> tests in the same :try block, if an early 'ok' test fails, all
> following tests won't be done, test coverage is reduced, and
> valuable information about test outcomes is missing from error
> reports.
> )
>
> Now I see: the most succinct way with full coverage is to do tests that
> are expected to pass outside of a :try block and without a following
> message, and tests that are expected to fail inside a :try block for
> each of them.
>
> Thanks!
>
> I just changed this test for the next patch round.
>
>> >> > +: $put =string(extend(d, {'x': 'new'}))
>> >> > +: $put ='did extend() with new item'
>> >> > +:catch
>> >> > +: $put =v:exception[:14]
>> >> > +:endtry
>> >> > +:$put =string(d)
>> >> > +:"
>> >>
>> >> Here some crucial tests are missing:
>> >>
>> >> 1. `extend(d, {'a': 456, 'x': 'new'})` without try/catch.
>> >> 2. Same in a try/catch.
>> >> 3. Both repeated with a second argument to `extend` where `keys(arg)`
>> >> returns `[old_key1, new_key2, old_key3]` (this is needed to make sure
>> >> that all old keys are modified).
>> >>
>> >> Unless it is possible for `extend` to modify *all* old keys regardless
>> >> the internal order I would prefer the current behaviour where you at
>> >> least always know that nothing will be modified.
>> >
>> > I sent a new patch (in a different mail) that changes all of the
>> > pre-existing and added error-on-exit behavior into all-or-nothing
>> > behavior, except for the out of memory case (which remains unchanged at
>> > continue-on-error). Please see my comments and the patch there.
>> >
>> > The patch includes a new test for all-or-nothing semantics, as you
>> > described in 3. above.
>> >
>> > Could you explain what tests 1. and 2. are meant to test?
>>
>> If you were going to use not all-or-nothing, but continue-on-error
>> they were testing how you continue on error specifically (that you do
>> this regardless of :try).
>
> I see.
>
> Please note that my original patch did not change the error handling
> mode, and was not intended to. That is why the purpose of these tests
> did not become clear without explanation.
>
> But the error handling mode of course is a matter of concern, as the
> patch introduces a new error condition that is potentially more likely
> to trigger in production code, *if* someone uses user locks in
> production code (which I still have difficulties to imagine), rather
> than for debugging.
>
> A test like this for continue-on-error behavior is now in the next
> version of the 'RFC' variant of the patch.
>
>> [...]
>> >> Test should also test the order of the `keys()` output to make sure
>> >> that it will be modified once changes that affect internal hash
>> >> ordering are pushed.
>> >
>> > For the change on extend() locks, what would testing for changes in hash
>> > ordering achieve?
>>
>> That if you have continue-on-error you *do* have continue-on-error.
>> Tests for changes in hash ordering are needed to check that test is
>> testing what it was expected to test: [existing, new, existing] in a
>> second dictionary makes sure that both existing keys are modified even
>> if error occures in between. If you do not check the ordering you
>> cannot know whether you are testing [existing, new, existing] or
>> [existing, existing, new] (which is useless because it will emit the
>> same result for both continue-on-error and exit-on-error).
>
> Thanks!
>
> In the meantime, I more or less got what you meant, and updated the
> patch.
>
> It is good to test this explicitly, so that this property cannot be
> overlooked, even though a hash ordering change is likely to be noticed
> anyway: hash items are printed or stringified in hash order, so that
> probably all tests that print dictionaries need to be updated then.
This is why I actually use either single-item dictionaries or
`string(sort(items(dictionary)))` to stringify dictionary when I need
this.
>
>> > We know that extend()'s current exit-on-error behavior depends on hash
>> > ordering. Of course, when the hash ordering changes, the item that
>> > causes an exit-on-error may come sooner or later. But this is not a
>> > regression.
>>
>> It is a regression *of the test itself*, not the regression in the code.
>>
>> If you prefer exit-on-error like it is currently doing [existing, new,
>> existing] will fail if behaviour will change compared to both
>> continue-on-error and all-or-nothing. [new, existing, existing] does
>> not allow to distinguish all-or-nothing and exit-on-error. [existing,
>> existing, new] does not allow to distinguish exit-on-error and
>> continue-on-error. So you need to check the ordering to make sure your
>> test is testing what it is expected to test.
>>
>> In the test in the next message you should check that 'c' is both not
>> the first and not the last key, not just not the first.
>
> Yes, with the attention that error handling modes get, we should test
> that they work as intended, and consequently also that the tests work as
> intended.
>
> An updated test is now in the next version of the 'RFC' variant of the
> patch.
>
> --
> Olaf Dabrunz (oda <at> fctrace.org)
>
> --
> --
> You received this message from the "vim_dev" maillist.
> Do not top-post! Type your reply below the text you are replying to.
> For more information, visit http://www.vim.org/maillist.php
>
> ---
> You received this message because you are subscribed to the Google Groups
> "vim_dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to vim_dev+unsubscr...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
--
--
You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php
---
You received this message because you are subscribed to the Google Groups
"vim_dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to vim_dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
