react: check callback values after multiple value changes

[cmccandless]

Depending on the language and implementation, a possible solution for callback values is to simply append the new value to the callback's value list. This is incorrect behavior, but no test verifies that the callback only contains 1 value per input. This PR adds a test that does just that.

[petertseng]

a possible solution for callback values is to simply append the new value to the callback's value list

I'd like to ask for clarification from you on this point. "Callback values" is not something that I perceive https://github.com/exercism/problem-specifications/blob/master/exercises/react/description.md to be asking for. From the point of view of the implementation that the student should provide, all that it knows about callbacks is that they should be called. How the test runner verifies that callbacks have been called is not something a submitted implementation should be aware of, therefore I was unsure what "solution to callback values" means. Since I had trouble understanding that, that makes it hard to understand the purpose of the PR. Would you explain?

but no test verifies that the callback only contains 1 value per input

I need some help understanding. An excerpt from "compute cells fire callbacks":

          {   
            "type": "set_value",
            "cell": "input",
            "value": 3
          },  
          {   
            "type": "expect_callback_values",
            "callback": "callback1",
            "values": [4] 
          } 

To me, this verifies that callbacks are only called with one value per change. A single change happened (to change the input from its initial value of 1 to a new value of 3), and it should result in the callback being called with 4 exactly one. It should not be called zero times, or twice, etc.

Without help on these two questions, I can't currently understand why these this test is necessary. I can see that there is a consistent way to change how expect_callback_values should be interpreted by a test generator or a person manually interpreting this test file to make this work, but not why it is necessary. Could I ask for an example of an incorrect solution that a student could submit that would pass all other tests but fail this new test (thus necessitating it)?

[cmccandless]

@petertseng

Could I ask for an example of an incorrect solution that a student could submit that would pass all other tests but fail this new test (thus necessitating it)?

Certainly. Currently exercism/python#1346 is refactoring the react exercise to better match the canonical data, and this iteration of the new example solution would produce the following failure for the proposed test:

expect_callback_values for callback1:
expected: [4]
actual: [3, 4]

In this implementation, cells "update" callbacks by appending their new value, but the old value is never removed. Thus, after the first change, the callback values contain the expected [3], but after the second change, the first updated value is not removed, so the new set of callback values contains both the previous value of 3 and the new value 4.

[petertseng]

Excellent, thank you for the example!

I'd like to discuss https://github.com/mRcfps/python/blob/bc8983225d6ff615b4adc089c6c0b5ba9a1e8b32/exercises/react/example.py#L59-L61 of the example solution linked. Reproduced here, it is:

class Callback(object):
    def __init__(self):
        self._values = []

I argue that this is not a https://en.wikipedia.org/wiki/Callback_(computer_programming). A callback should just be a plain old function, and the only thing a Cell should do with a callback is to call it like any other function. So to me that means a callback should have no memory of what values it has been called with. The only thing it can do is be called. So it should have no values property.

[petertseng]

I'd like to offer that perhaps canonical-data.json is unnecessarily confusing, with the terminology expect_callback_values to look too similar to expect_cell_value while not implying the same thing about the API (cells do have a value property, but callbacks do not have a values property). I have some suggestions to remedy this; one or more may be chosen.

• Rename expect_callback_values to some indicative name such as expect_callback_to_have_been_called_with_values
• Remove expect_callback_values entirely, instead expressing it as an expected_callbacks k/v pair of any operation with type set_value, since that is the most precise time we can expect a callback to be called. Be mindful of any increased burden on testing code if this approach is to be taken.
• Your suggestion here.

[cmccandless]

I'd like to offer that perhaps canonical-data.json is unnecessarily confusing

I agree it could use some reworking.

• Rename expect_callback_values to some indicative name such as expect_callback_to_have_been_called_with_values

I'm not sure this would solve the issue, as contributors are still likely to interpret this as an operation (still listed under "operations").

• Remove expect_callback_values entirely, instead expressing it as an expected_callbacks k/v pair of any operation with type set_value, since that is the most precise time we can expect a callback to be called. Be mindful of any increased burden on testing code if this approach is to be taken.

I like this better. However, there is still the requirement that callbacks are only run once, even if multiple dependencies change. There must be some way to express this in canonical-data.json.

[petertseng]

there is still the requirement that callbacks are only run once, even if multiple dependencies change. There must be some way to express this in canonical-data.json.

Yes, good point. I expect it to be handled in a way similar to this (demonstrating how an existing test would be changed):

    {
      "description": "callbacks should only be called once even if multiple dependencies change",
      "comments": [
        "Some incorrect implementations call a callback function too early,",
        "when not all of the inputs of a compute cell have propagated new values."
      ],
      "property": "react",
      "input": {
        "cells": [
          {
            "name": "input",
            "type": "input",
            "initial_value": 1
          },
          {
            "name": "plus_one",
            "type": "compute",
            "inputs": ["input"],
            "compute_function": "inputs[0] + 1"
          },
          {
            "name": "minus_one1",
            "type": "compute",
            "inputs": ["input"],
            "compute_function": "inputs[0] - 1"
          },
          {
            "name": "minus_one2",
            "type": "compute",
            "inputs": ["minus_one1"],
            "compute_function": "inputs[0] - 1"
          },
          {
            "name": "output",
            "type": "compute",
            "inputs": ["plus_one", "minus_one2"],
            "compute_function": "inputs[0] * inputs[1]"
          }
        ],
        "operations": [
          {
            "type": "add_callback",
            "cell": "output",
            "name": "callback1"
          },
          {
            "type": "set_value",
            "cell": "input",
            "value": 4,
            "expect_callbacks": {
              "callback1": 10
            },
          }
        ]
      },
      "expected": {}
    }

Each value in expect_callbacks is a single value, not a list, because callbacks should be called once (if the value of its output cell changes as a result of that set_value on the input cell) or zero times (if the value of its output cell doesn't change). Being called more than once is expected to never happen. The fact that it is not a list is how I intend to express this in the JSON file.

To be discussed: That shows how we express the expectation that a callback is called. I wonder how we
express an expectation that a callback (call it callbackN) that should not be called. A few options here:

• Put it in expect_callbacks something like "expect_callbacks": {"callbackN": null}
• Have it be missing from expect_callbacks, which requires that we define that a callback will be named in expect_callbacks for a set_value if and only if it will be called as a result of that set_value.
• Have an explicit "expect_callbacks_not_to_be_called": ["callbackN"]

[petertseng]

I figured out an angle under which this PR is helpful: As a stepping stone that provides a smaller incremental step.

The test named "callbacks can be added and removed" implicitly tests that the same callback can fire multiple times (once for each of two successive set_value), but there was no explicit test for this fact. Adding the explicit test will be a good steppling stone. All existing submitted solutions should pass but it should give a little more help to new solutions being written.

I think this test case is already placed well to serve the purpose I propose: It is after "compute cells fire callbacks" and before "callbacks can be added and removed". I have no opinion on whether it should go before or after "callback cells only fire on change".

The problem of having to explicitly define expect_callback_values is solved by #1196, but #1196 is deliberately a separate PR. It doesn't matter to me which order the PRs get merged.

This is the first time in this file's history where check_callback_values s used twice on the same callback ID and expects values both times. I request that one of two choices be taken to deal with this:

• If the second expectation remains [4], let's explicitly note in the definition of expect_callback_values at the top of the file that these are the values since the last expect_callback_values operation.
• Or, let's explicitly note in the definition of expect_callback_values above that these are the values the callback has ever been called with, regardless of any previous expect_callback_values operation, in which case the second expectation should change to [3, 4].

[cmccandless]

@petertseng

If the second expectation remains [4], let's explicitly note in the definition of expect_callback_values at the top of the file that these are the values since the last expect_callback_values operation.

I like this, and have added a commit with this change.

[petertseng]

If you don't mind, may I bikeshed this? Given that

1. I believe this test is most useful for demonstrating a callback being called multiple times
2. Given my interpretation of what a callback is, they do not contain values

I would prefer a description that doesn't use the word "contain". I think I would be happy with "callbacks can be called twice", "callbacks can be called multiple times". If wanting to stress the fact that new values will be present, perhaps "callbacks can be called multiple times with new values".

[cmccandless]

Fair enough. I'll work on it.

[petertseng]

Yup I'm down with "do not report already reported values"

I prefer commits be squashed when merging.