react: Express callback expectations precisely after each `set_value` by petertseng · Pull Request #434 · exercism/rust

petertseng · 2018-02-23T02:57:25Z

react: Express callback expectations precisely after each set_value

Problem statement:

Consider a test with two set_value calls and which expects that a
callback has, ultimately, been called with the two values, one for each
set_value.

The tests currently do not check that one value was added during each
set_value call. For all we know, maybe an implementation:

magically manages to predict the future and calls the callback twice
on the first set_value call, with the correct value.
calls the callback zero times on the first set_value call, but twice
on the second set_value call.

To more precisely define the set_value expectations, this commit uses
a Cell-based implementation to test callbacks.

Discussion:

Is this clearer to students than the old vector-based approach? It is
certainly true that the expectations are closer to the line of code that
could cause the expectations to fail; each set_value now shows exactly
what callback calls are expected as a result of it. I would argue this
is does increase test clarity.

Some questions I will have:

Is there any potential for student confusion if they were to read the
implementation of the Expector class? Any way to alleviate that
confusion?
A better name for Expector. It was chosen arbitrarily simply because
it is something that expects.
Whether this can in fact be done without Cell. I tried my best but
could not find a way.

It behooves me to explain why I could not. If the reason is already obvious to you because of your knowledge of Rust, you can probably skip this part, then.

If the callback mutably borrows something (say, to record what value it was called with), the rest of the test code cannot borrow that same thing to check that the values are correct. Same if we try to go vice-versa: If the test code mutably borrows something to signal to the callback "I am going to call set_value now so it is safe for you to let one more value through", the callback cannot have immutably borrowed it to check that signal. So it seems the only sharing the two can do is through both of them immutably borrowing something.

If they must both immutably borrow something, it seems that at least one of them will nevertheless need to mutate something. ~~Thus, it seems to be the case that we must achieve dynamic (runtime-checked, instead of compile-time-checked) borrowing, by using a RefCell.~~
After experimentation, we can use a Cell, preferable because now there is no dynamic borrowing.

petertseng · 2018-02-23T03:17:18Z

The possible outcomes that I expect from this discussion are the two that might immediately come to mind.

Decide that this is an improvement, and use this implementation for all (or maybe only some?) of the tests in this file. I volunteer to make the change if this is so.
Decide that this is not an improvement, and close this PR after having reached this conclusion.

petertseng · 2018-02-23T03:53:05Z

This tells me that this is possible in Rust, which allays my fears about making such tests canonical as I suggest in exercism/problem-specifications#1194 (comment) .I will probably decide it's better to be precise in the canonical data, even if the Rust track chooses not to do this.

coriolinus · 2018-02-23T09:53:55Z

I need some more time to think this over; I do not yet have an opinion about this.

I do, however, have a bit of dogshedding: it wasn't obvious to me at first what the distinction between have_value and value were. It feels to me like that would idiomatically be expressed in terms of an Option; was there a reason not to simply write the following?

struct Expector {
    value: RefCell<Option<isize>>,
}

petertseng

guess that worked, and opens up a few possibilities of wanting to save lines

petertseng · 2018-02-23T10:37:25Z

-        assert_eq!(*self.value.borrow(), v, "Callback was called with incorrect value");
-        *self.have_value.borrow_mut() = false;
+        assert_ne!(*self.value.borrow(), None, "Callback was not called, but should have been");
+        assert_eq!(*self.value.borrow(), Some(v), "Callback was called with incorrect value");


I'd consider using https://doc.rust-lang.org/std/cell/struct.RefCell.html#method.replace in this line to remove the need for the line below it.

Yes, I think that would be natural.

petertseng · 2018-02-23T10:37:46Z

-        assert!(*self.have_value.borrow(), "Callback was not called, but should have been");
-        assert_eq!(*self.value.borrow(), v, "Callback was called with incorrect value");
-        *self.have_value.borrow_mut() = false;
+        assert_ne!(*self.value.borrow(), None, "Callback was not called, but should have been");


not necessary, given the below line, but maybe helpful for a better error message???

Sure; to present a human-friendly error message in test code is definitely worth an extra line and a single repeated branch.

petertseng · 2018-02-23T10:38:17Z

-        assert!(!*self.have_value.borrow(), "Callback was called too many times");
-        *self.value.borrow_mut() = v;
-        *self.have_value.borrow_mut() = true;
+        assert_eq!(*self.value.borrow(), None, "Callback was called too many times; can't be called with {}", v);


also possible use of https://doc.rust-lang.org/std/cell/struct.RefCell.html#method.replace

petertseng · 2018-02-26T00:46:41Z

 }

+use std::cell::RefCell;
+struct Expector {


CallbackRecorder, maybe.

petertseng · 2018-03-01T00:05:29Z

Ah, as much as I think this is a cool thing to have, I see that https://blog.rust-lang.org/2018/02/15/Rust-1.24.html tells me replace was only stabilised in 1.24. Caution may be needed.

petertseng · 2018-03-01T00:09:38Z

Now that I understand CellandRefCell, I understand now that I can use Cell instead of RefCell. I think Cell's operations have been stable for longer (https://doc.rust-lang.org/std/cell/struct.Cell.html#method.replace has 1.17.0 by the name so I assume that's the release where it was made stable)

and https://blog.rust-lang.org/2017/04/27/Rust-1.17.html confirms the thought about Cell replace 1.17.

petertseng · 2018-03-01T00:17:44Z

 }

+struct Expector {
+    value: std::cell::Cell<Option<isize>>,


I'm only doing this because I thought If I just use cell here (with a use std::cell::Cell before, of course), what if a submitted implementation (which we use react::*) has a Cell ? will the conflicting names named in a use cause problems? I admit I never tried.

It does not cause problems if Cell is specifically named:

mod m1 { #[derive(Debug)] pub struct A { pub a: usize, } } mod m2 { #[derive(Debug)] pub struct A { pub a: usize, } } use m1::*; use m2::A; fn main() { println!("Hello, world! {:?}", A{a: 1}); }

So, could go back to use std::cell::Cell, but perhaps I'll leave it this way for clarity in case someone doesn't know the rules. I didn't know off the top of my head.

coriolinus · 2018-03-09T20:41:35Z

I've finally had the chance to think about this for long enough to understand it. I think these changes improve the exercise:

expected behavior is tested more precisely
the mechanism of tests is a bit more elegant
it gives students a working example of how (and when) to use Cell.

I'm not approving in the Github sense largely because this PR is a demo, not yet complete. It doesn't apply to all tests.

The most compelling objection is that this increases the cognitive load for students who want to understand how the tests work. My response to that is that this is a level 10 exercise, one of only three in the Rust track IIRC. Students who attempt this exercise are expected to be comfortable with advanced idioms, and they have the opportunity to learn something useful by studying this construction.

Thank you, @petertseng, for coming up with this; it's very cool. I like it.

petertseng · 2018-03-10T01:24:02Z

The most compelling objection is that this increases the cognitive load for students who want to understand how the tests work.

My response here is that I hope the names of the functions should make this clear, but I also propose to add some comments to help.

Commits coming that will use this for all the callback tests; I'll rely on GitHub's standard notifications for that. I do propose that I squash all these commits when merging.

petertseng · 2018-03-10T01:28:43Z

A note I've discovered: All let cb = MUST come before let mut reactor =, otherwise there will be an error like the following:

error[E0597]: `cb` does not live long enough
   --> tests/react.rs:133:46
    |
133 |     assert!(reactor.add_callback(output, |v| cb.callback_called(v)).is_ok());
    |                                          --- ^^ borrowed value does not live long enough
    |                                          |
    |                                          capture occurs here
...
137 | }
    | - borrowed value dropped before borrower
    |
    = note: values in a scope are dropped in the opposite order they are created

So we'll have to live with the fact that the let cb will be at the top, rather than, say, right before the callback gets added.

petertseng · 2018-03-10T01:32:42Z

Feel free to ask for extra newlines to be added to make the tests easier to read.

petertseng · 2018-03-10T02:32:54Z

It is good manners for me to say that the improvement of more precisely saying which callbacks should be called for each set_value is not Rust-specific, and that exercism/problem-specifications#1196 will apply it to the canonical data.

petertseng · 2018-03-13T00:38:36Z

After rebasing for #454 I believe that git diff 4fc6e7c e925ff2 -- exercises/react told me that the only changes are to remove the useless vec!.

**Problem statement**: Consider a test with two `set_value` calls and which expects that a callback has, ultimately, been called with the two values, one for each `set_value`. The tests currently do not check that one value was added during each `set_value` call. For all we know, maybe an implementation: * magically manages to predict the future and calls the callback twice on the first `set_value` call, with the correct value. * calls the callback zero times on the first `set_value` call, but twice on the second `set_value` call. To more precisely define the `set_value` expectations, this commit uses a `Cell`-based implementation to test callbacks.

petertseng · 2018-03-16T01:51:25Z

To avoid confusion on what I intend the final product's commit message to be (and to support another upcoming change to react), I've squashed down to one commit. If anyone wishes to see the old history, you may see at https://github.com/petertseng/exercism-rust/commits/e925ff28cba1abfc41956bb0382f265a886130ac. I do not believe any of that is worth keeping as separate commits since they were all exploratory.

Maybe less confusion for students; they don't need to worry about callbacks until they actually run into those tests.

petertseng · 2018-03-19T05:04:52Z

As you can see, I am proposing moving CallbackRecorder to right before the first test involving callbacks, instead of at the very top of the file. Let me know if that's better.

coriolinus

Notwithstanding my quibble about the documentation block, I think this looks good. Positioning the definition before its first use instead of before the first test improves usability for a student who is actually unlocking the tests one at a time.

coriolinus · 2018-04-04T15:18:01Z

+    value: std::cell::Cell<Option<isize>>,
+}
+
+/// A CallbackRecorder helps tests whether callbacks get called correctly.


I do not know whether doccomments are parsed when they precede the impl block instead of the struct block. The documentation on documentation doesn't say. That said, my intuition is that this comment should probably move to the struct definition instead of the impl.

If it can be shown that cargo doc produces correct output when the documentation is attached to the impl block, I will rescind this comment.

coriolinus · 2018-04-04T16:20:34Z

I will not merge now, so as to allow potential movement of the documentation block.

petertseng · 2018-04-04T18:23:49Z

I am adding a commit to move the doc comment. I intend that these commits should all be squashed and will do so if the CI passes.

petertseng · 2018-04-04T18:31:07Z

Confirmed via cargo new a and the following in a/src/lib.rs

pub struct hi {
    a: usize,
}

/// hi is an impl
impl hi {
    fn a() -> usize {
        5
    }
}

cargo doc has no doc for a

(but it does if you put /// hi is a struct above the struct)

petertseng · 2018-04-04T18:33:07Z

correction: the index has no doc for hi.

The specific documentation page for hi, however, does say, under the impl hi section, "hi is an impl".

For our case, it is better to put the doc on the struct.

petertseng · 2018-04-04T18:40:56Z

Although I did not rebase this, the stub has not changed and by pushing a new commit this had the effect of making Travis test the merge result (https://travis-ci.org/exercism/rust/jobs/362275367 tested commit 330f7d3 which merged 664236c, my commit, into ccc6f0fe471f68034ff6dd9f68401db027b2094dm, current master)

petertseng · 2018-04-04T23:24:16Z

+    assert!(reactor.add_callback(output, |v| cb2.callback_called(v)).is_ok());
+
+    assert!(reactor.set_value(input, 31).is_ok());
+    cb1.expect_to_have_been_called_with(32);


Note as to how this applies to other languages:

Other languages would have been perfectly fine to use their language's equivalent of the old vector-based implementation, because they are free to check that the vector == [32] here. It was solely because I was unable to to do in Rust that I chose to use a Cell.

Other languages need not use their equivalent of this Cell-based implementation, but may still choose to if they feel it makes their tests more descriptive.

What all languages can indeed benefit from is testing that the callbacks were called with 32 at this point in the program (instead of waiting until the end), no matter how that is implemented.

petertseng commented Feb 23, 2018

View reviewed changes

petertseng commented Feb 26, 2018

View reviewed changes

petertseng commented Mar 1, 2018

View reviewed changes

petertseng changed the title ~~react: Experiment with RefCell~~ react: Express callback expectations precisely after each set_value Mar 10, 2018

petertseng mentioned this pull request Mar 13, 2018

Use &[T] instead of &Vec<T> where possible #454

Merged

This was referenced Mar 16, 2018

react: return domain-specific errors (DO NOT MERGE, reference for #434 #464 merge ONLY) #463

Closed

react: return domain-specific errors #464

Merged

react: Investigate more type safety for input vs compute cells #473

Closed

react: Move CallbackRecorder before the first callback test

3d50e77

Maybe less confusion for students; they don't need to worry about callbacks until they actually run into those tests.

coriolinus approved these changes Apr 4, 2018

View reviewed changes

react: document the struct, not the impl

664236c

petertseng merged commit a61ec4d into exercism:master Apr 4, 2018

petertseng deleted the react-experiment branch April 4, 2018 18:41

petertseng commented Apr 4, 2018

View reviewed changes

petertseng mentioned this pull request Jan 22, 2019

React exercism/ruby#923

Closed

Conversation

petertseng commented Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petertseng commented Feb 23, 2018

Uh oh!

petertseng commented Feb 23, 2018

Uh oh!

coriolinus commented Feb 23, 2018

Uh oh!

petertseng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petertseng commented Mar 1, 2018

Uh oh!

petertseng commented Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coriolinus commented Mar 9, 2018

Uh oh!

petertseng commented Mar 10, 2018

Uh oh!

petertseng commented Mar 10, 2018

Uh oh!

petertseng commented Mar 10, 2018

Uh oh!

petertseng commented Mar 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petertseng commented Mar 13, 2018

Uh oh!

petertseng commented Mar 16, 2018

Uh oh!

petertseng commented Mar 19, 2018

Uh oh!

coriolinus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coriolinus commented Apr 4, 2018

Uh oh!

petertseng commented Apr 4, 2018

Uh oh!

petertseng commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petertseng commented Apr 4, 2018

Uh oh!

petertseng commented Apr 4, 2018

Uh oh!

petertseng Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

petertseng commented Feb 23, 2018 •

edited

Loading

petertseng commented Mar 1, 2018 •

edited

Loading

petertseng commented Mar 10, 2018 •

edited

Loading

petertseng commented Apr 4, 2018 •

edited

Loading

petertseng Apr 4, 2018 •

edited

Loading