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.
> > 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.
