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.