Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
19839002:
Improve test coverage of reflective invocation. (Closed)
|
Created:
7 years, 5 months ago by rmacnak Modified:
7 years, 4 months ago CC:
reviews_dartlang.org, Michael Lippautz (Google) Visibility:
Public. |
DescriptionPatch Set 1 : #Patch Set 2 : #
Total comments: 18
Patch Set 3 : #
Total comments: 3
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 24
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #
Total comments: 1
Patch Set 9 : #Messages
Total messages: 20 (0 generated)
Peter, on dart2js it failed at getField(#staticGetter)
https://chromiumcodereview.appspot.com/19839002/diff/8001/runtime/lib/mirrors... File runtime/lib/mirrors_impl.dart (right): https://chromiumcodereview.appspot.com/19839002/diff/8001/runtime/lib/mirrors... runtime/lib/mirrors_impl.dart:232: throw "setter argument ($value) must be a simple value or InstanceMirror"; Long line. https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/lib.status File tests/lib/lib.status (right): https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/lib.statu... tests/lib/lib.status:7: mirrors/invoke_test: Fail Please add a bug number. https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/mirrors/i... File tests/lib/mirrors/invoke_test.dart (right): https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/mirrors/i... tests/lib/mirrors/invoke_test.dart:13: var field; Weird indentation. https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/mirrors/i... tests/lib/mirrors/invoke_test.dart:62: // Expect does not throw. What does this mean? https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/mirrors/i... tests/lib/mirrors/invoke_test.dart:69: result = cm.invoke(const Symbol('doesntExist'),[3,4]); Weird indentation. https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/mirrors/i... tests/lib/mirrors/invoke_test.dart:71: } catch(exception) {} This doesn't work. Also, NEVER have an empty catch block, at least write a comment about why it is appropriate to ignore the error. Take a look at Expect.throws. https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/mirrors/i... tests/lib/mirrors/invoke_test.dart:73: result = cm.invoke(const Symbol('staticFunction'),[3]); // Wrong arity. Weird indention. https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/mirrors/i... tests/lib/mirrors/invoke_test.dart:75: } catch(exception) {} This doesn't work. https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/mirrors/i... tests/lib/mirrors/invoke_test.dart:77: // ClassMirror invokeGetter Seem to be a systemic problem below: Weird indentation and bad tests of exceptions.
https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/lib.status File tests/lib/lib.status (right): https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/lib.statu... tests/lib/lib.status:7: mirrors/invoke_test: Fail On 2013/07/20 16:34:59, ahe wrote: > Please add a bug number. Done. https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/mirrors/i... File tests/lib/mirrors/invoke_test.dart (right): https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/mirrors/i... tests/lib/mirrors/invoke_test.dart:13: var field; On 2013/07/20 16:34:59, ahe wrote: > Weird indentation. Oops, Sublime normally converts my tabs to spaces. My guess is it didn't know which rule to use initially because this was a new file. https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/mirrors/i... tests/lib/mirrors/invoke_test.dart:62: // Expect does not throw. On 2013/07/20 16:34:59, ahe wrote: > What does this mean? We expect setField(#doesntExist) to complete without raising an exception. It produces no side-effect and the return value of setField is ill-defined. https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/mirrors/i... tests/lib/mirrors/invoke_test.dart:71: } catch(exception) {} On 2013/07/20 16:34:59, ahe wrote: > This doesn't work. Also, NEVER have an empty catch block, at least write a > comment about why it is appropriate to ignore the error. > > Take a look at Expect.throws. This works on the VM. In any case, converted to Expect.throws. https://chromiumcodereview.appspot.com/19839002/diff/13001/tests/lib/mirrors/... File tests/lib/mirrors/invoke_test.dart (right): https://chromiumcodereview.appspot.com/19839002/diff/13001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:119: (e) => e is MirroredCompilationError, Passing a predicate for this is gross, but we don't have first-class classes.
Gilad, could you please take a look at this test. I don't think MirroredCompilationError is the right thing to do. I believe we agreed that calling setField is the same as performing an assignment in the language specification, and a getField is the same as a property access in the language specification. https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/mirrors/i... File tests/lib/mirrors/invoke_test.dart (right): https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/mirrors/i... tests/lib/mirrors/invoke_test.dart:62: // Expect does not throw. On 2013/07/22 18:40:48, Ryan Macnak wrote: > On 2013/07/20 16:34:59, ahe wrote: > > What does this mean? > > We expect setField(#doesntExist) to complete without raising an exception. It > produces no side-effect and the return value of setField is ill-defined. Please add the comment *above* the line to which it applies. Also, write a complete sentence. The return value of setField is defined to be the an instance mirror on the second argument. This is what the language specification says. https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/mirrors/i... tests/lib/mirrors/invoke_test.dart:71: } catch(exception) {} On 2013/07/22 18:40:48, Ryan Macnak wrote: > On 2013/07/20 16:34:59, ahe wrote: > > This doesn't work. Also, NEVER have an empty catch block, at least write a > > comment about why it is appropriate to ignore the error. > > > > Take a look at Expect.throws. > > This works on the VM. In any case, converted to Expect.throws. What I meant is: You could do absolutely anything in the try block (as long as it isn't a compile-time error). If an error is thrown the test "pass", if no error is thrown the test "pass". https://chromiumcodereview.appspot.com/19839002/diff/13001/tests/lib/mirrors/... File tests/lib/mirrors/invoke_test.dart (right): https://chromiumcodereview.appspot.com/19839002/diff/13001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:119: (e) => e is MirroredCompilationError, On 2013/07/22 18:40:48, Ryan Macnak wrote: > Passing a predicate for this is gross, but we don't have first-class classes. I am rather sure that MirroredCompilationError is wrong. Accessing a non-existing field is a NoSuchMethodError, not a compilation error. Also, if a compilation error occurs, the current isolate is suspended. MirroredCompilationError is only relevant for remote isolates.
https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/mirrors/i... File tests/lib/mirrors/invoke_test.dart (right): https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/mirrors/i... tests/lib/mirrors/invoke_test.dart:62: // Expect does not throw. On 2013/07/22 19:29:49, ahe wrote: > On 2013/07/22 18:40:48, Ryan Macnak wrote: > > On 2013/07/20 16:34:59, ahe wrote: > > > What does this mean? > > > > We expect setField(#doesntExist) to complete without raising an exception. It > > produces no side-effect and the return value of setField is ill-defined. > > Please add the comment *above* the line to which it applies. Also, write a > complete sentence. Done. > The return value of setField is defined to be the an instance mirror on the > second argument. This is what the language specification says. Currently the VM will give an instance mirror on the result of the setter, which isn't necessarily the setter's argument. I will address this in another CL. https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/mirrors/i... tests/lib/mirrors/invoke_test.dart:71: } catch(exception) {} On 2013/07/22 19:29:49, ahe wrote: > On 2013/07/22 18:40:48, Ryan Macnak wrote: > > On 2013/07/20 16:34:59, ahe wrote: > > > This doesn't work. Also, NEVER have an empty catch block, at least write a > > > comment about why it is appropriate to ignore the error. > > > > > > Take a look at Expect.throws. > > > > This works on the VM. In any case, converted to Expect.throws. > > What I meant is: You could do absolutely anything in the try block (as long as > it isn't a compile-time error). If an error is thrown the test "pass", if no > error is thrown the test "pass". Ah, you mean it doesn't properly assert both control flows. Now I see. https://chromiumcodereview.appspot.com/19839002/diff/13001/tests/lib/mirrors/... File tests/lib/mirrors/invoke_test.dart (right): https://chromiumcodereview.appspot.com/19839002/diff/13001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:119: (e) => e is MirroredCompilationError, On 2013/07/22 19:29:49, ahe wrote: > On 2013/07/22 18:40:48, Ryan Macnak wrote: > > Passing a predicate for this is gross, but we don't have first-class classes. > > I am rather sure that MirroredCompilationError is wrong. > > Accessing a non-existing field is a NoSuchMethodError, not a compilation error. > > Also, if a compilation error occurs, the current isolate is suspended. > MirroredCompilationError is only relevant for remote isolates. Agreed, tracking as issue 11957.
Ryan can you separate this CL into two parts, the change in mirrors_impl.dart can go in as one CL and the test as another CL once all issues are resolved. https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/lib.status File tests/lib/lib.status (right): https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/lib.statu... tests/lib/lib.status:7: mirrors/invoke_test: Fail You need an issue for this...
On 2013/07/22 22:46:14, siva wrote: > Ryan can you separate this CL into two parts, > the change in mirrors_impl.dart can go in as one CL > and the test as another CL once all issues are resolved. > > https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/lib.status > File tests/lib/lib.status (right): > > https://chromiumcodereview.appspot.com/19839002/diff/8001/tests/lib/lib.statu... > tests/lib/lib.status:7: mirrors/invoke_test: Fail > You need an issue for this... Pulled the fix out into a new CL.
Very nice, but there are still some issues. I'm mostly worried about not chaining the futures. But I'm also concerned about not testing the return value of setField. https://chromiumcodereview.appspot.com/19839002/diff/13003/tests/lib/mirrors/... File tests/lib/mirrors/invoke_test.dart (right): https://chromiumcodereview.appspot.com/19839002/diff/13003/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:42: Expect.fail("Value expected"); How about: Expect.fail("Value expected. ($e)"); https://chromiumcodereview.appspot.com/19839002/diff/13003/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:47: shouldYieldError(future, errorPredicate, reason){ Missing space between ) and {. https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... File tests/lib/mirrors/invoke_test.dart (right): https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:89: Expect.equals('set foo', c.field); Could you also test that getField still return the expected value. This can be rather important for dart2js as it is too easy to accidentally access (or create) the wrong property on a JS object. https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:94: im.setField(const Symbol('doesntExist'), 'bar'); You could test that it returns 'bar'. https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:118: Expect.equals('sset sfoo', C.staticField); Again, it would be nice if you also test that getField still returns the expected values. https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:142: Expect.equals(':)', result.reflectee); :) https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:161: Expect.equals('lset lfoo', libraryField); Again, it would be nice if you test getField still returns what you expect. https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:169: testAsync() { I think you need to chain most of the futures in this test. https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:208: shouldYieldValue(future).then((result) { Check result. https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:211: shouldYieldValue(future2).then((result) { Check result. https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:214: shouldYieldValue(future3).then((result) { Check result. https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:220: shouldYieldValue(future); I think you should test the returned value. https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:256: shouldYieldValue(future2).then((result) { Check result. https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:259: shouldYieldValue(future3).then((result) { Check result. https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:301: future = lm.invokeAsync(const Symbol('libraryFunction'),[':']); // Wrong arity. Long line. https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:319: shouldYieldValue(future).then((result) { Check result. https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:322: shouldYieldValue(future2).then((result) { Check result. https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:325: shouldYieldValue(future3).then((result) { Check result.
https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... File tests/lib/mirrors/invoke_test.dart (right): https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:94: im.setField(const Symbol('doesntExist'), 'bar'); On 2013/07/24 09:43:35, ahe wrote: > You could test that it returns 'bar'. Pending https://codereview.chromium.org/20066004/ https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:142: Expect.equals(':)', result.reflectee); On 2013/07/24 09:43:35, ahe wrote: > :) :D https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:169: testAsync() { On 2013/07/24 09:43:35, ahe wrote: > I think you need to chain most of the futures in this test. Something like shouldYieldValue(m.somethingAync).then((result){ checkResult shouldYieldValue(m.somethingElseAsync) }).then((result){ checkResult shouldYieldValue(m.somethingElseAsync) }).then((result){ ... }) ? What about errors? Can one chain a then() after a catchError()?
On 2013/07/24 17:03:18, Ryan Macnak wrote: > https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... > File tests/lib/mirrors/invoke_test.dart (right): > > https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... > tests/lib/mirrors/invoke_test.dart:94: im.setField(const Symbol('doesntExist'), > 'bar'); > On 2013/07/24 09:43:35, ahe wrote: > > You could test that it returns 'bar'. > > Pending https://codereview.chromium.org/20066004/ > > https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... > tests/lib/mirrors/invoke_test.dart:142: Expect.equals(':)', result.reflectee); > On 2013/07/24 09:43:35, ahe wrote: > > :) > > :D > > https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... > tests/lib/mirrors/invoke_test.dart:169: testAsync() { > On 2013/07/24 09:43:35, ahe wrote: > > I think you need to chain most of the futures in this test. > > Something like > > shouldYieldValue(m.somethingAync).then((result){ > checkResult > shouldYieldValue(m.somethingElseAsync) > }).then((result){ > checkResult > shouldYieldValue(m.somethingElseAsync) > }).then((result){ > ... > }) > > ? What about errors? Can one chain a then() after a catchError()? Now checking results of setField and no longer checking for MirroredCompilationError.
https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... File tests/lib/mirrors/invoke_test.dart (right): https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:169: testAsync() { On 2013/07/24 17:03:19, Ryan Macnak wrote: > On 2013/07/24 09:43:35, ahe wrote: > > I think you need to chain most of the futures in this test. > > Something like > > shouldYieldValue(m.somethingAync).then((result){ > checkResult > shouldYieldValue(m.somethingElseAsync) > }).then((result){ > checkResult > shouldYieldValue(m.somethingElseAsync) > }).then((result){ > ... > }) > > ? What about errors? Can one chain a then() after a catchError()? Let's ask Florian for help.
https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... File tests/lib/mirrors/invoke_test.dart (right): https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:178: }); Florian points out that this would be safer: shouldYieldValue(future.then((result) { Expect.equals('2+4+8', result.reflectee); }));
DBC. https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... File tests/lib/mirrors/invoke_test.dart (right): https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:208: shouldYieldValue(future).then((result) { This can be written as: shouldYieldValue( im.setFieldAsync(const Symbol('setter'), 'foo').then((result) { Expect.equals('set foo', c.field); return im.setFieldAsync(const Symbol('field'), 'bar'); }).then((result) { Expect.equals('bar', c.field); return im.setFieldAsync(const Symbol('field'), im); }).then((result) { Expect.equals(c, c.field); })); https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:253: shouldYieldValue(future).then((result) { ditto (use implicit chaining). https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:318: future = lm.setFieldAsync(const Symbol('librarySetter'), 'lfoo'); same here (implicit chaining).
On 2013/07/25 12:42:58, floitsch wrote: > DBC. > > https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... > File tests/lib/mirrors/invoke_test.dart (right): > > https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... > tests/lib/mirrors/invoke_test.dart:208: shouldYieldValue(future).then((result) { > This can be written as: > shouldYieldValue( > im.setFieldAsync(const Symbol('setter'), 'foo').then((result) { > Expect.equals('set foo', c.field); > return im.setFieldAsync(const Symbol('field'), 'bar'); > }).then((result) { > Expect.equals('bar', c.field); > return im.setFieldAsync(const Symbol('field'), im); > }).then((result) { > Expect.equals(c, c.field); > })); > > https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... > tests/lib/mirrors/invoke_test.dart:253: shouldYieldValue(future).then((result) { > ditto (use implicit chaining). > > https://chromiumcodereview.appspot.com/19839002/diff/22001/tests/lib/mirrors/... > tests/lib/mirrors/invoke_test.dart:318: future = lm.setFieldAsync(const > Symbol('librarySetter'), 'lfoo'); > same here (implicit chaining). Re-wrote future chaining.
https://chromiumcodereview.appspot.com/19839002/diff/45001/tests/lib/mirrors/... File tests/lib/mirrors/invoke_test.dart (right): https://chromiumcodereview.appspot.com/19839002/diff/45001/tests/lib/mirrors/... tests/lib/mirrors/invoke_test.dart:188: shouldYieldValue(future.then((result) { I'm not sure about this pattern. You're not making sure that the closure is actually being called. I would be more comfortable with something like this: future.then(expectValue((result) { ...
On 2013/08/06 12:25:34, ahe wrote: > https://chromiumcodereview.appspot.com/19839002/diff/45001/tests/lib/mirrors/... > File tests/lib/mirrors/invoke_test.dart (right): > > https://chromiumcodereview.appspot.com/19839002/diff/45001/tests/lib/mirrors/... > tests/lib/mirrors/invoke_test.dart:188: shouldYieldValue(future.then((result) { > I'm not sure about this pattern. > > You're not making sure that the closure is actually being called. This ensures the closure is called provided the implementation of future chaining is correct. > I would be more comfortable with something like this: > > future.then(expectValue((result) { ... Here expectValue takes the when-closure instead of a future, so we wouldn't be able to test for errors and they would manifest as timeouts.
On 2013/08/06 17:08:20, Ryan Macnak wrote: > On 2013/08/06 12:25:34, ahe wrote: > > > https://chromiumcodereview.appspot.com/19839002/diff/45001/tests/lib/mirrors/... > > File tests/lib/mirrors/invoke_test.dart (right): > > > > > https://chromiumcodereview.appspot.com/19839002/diff/45001/tests/lib/mirrors/... > > tests/lib/mirrors/invoke_test.dart:188: shouldYieldValue(future.then((result) > { > > I'm not sure about this pattern. > > > > You're not making sure that the closure is actually being called. > > This ensures the closure is called provided the implementation of future > chaining is correct. > > > I would be more comfortable with something like this: > > > > future.then(expectValue((result) { ... > > Here expectValue takes the when-closure instead of a future, so we wouldn't be > able to test for errors and they would manifest as timeouts. You can also have shouldYieldValue take two arguments, the future, and the "then" action. Then shouldYieldValue can set things up correctly.
Revised
LGTM!
Message was sent while issue was closed.
Committed patchset #9 manually as r25826 (presubmit successful). |
