React tests #188
React tests #188
Conversation
The motivation is twofold: First, to reduce repetition in the code. Second, to provide the user the ability to see the expected value of the cell versus the observed value so the user has a better idea of what went wrong.
| } | ||
|
|
||
| // Compute cells can depend on other compute cells. | ||
| // Compute 2 cells can depend on compute 1 cells. | ||
| func TestTopology(t *testing.T) { |
There was a problem hiding this comment.
Perhaps a name change is in order now, since there are now multiple topologies as opposed to before when this was the only topology test. Perhaps TestCompute2Diamond because the topology is kind of in a diamond shape. Suggestions very welcome. Or if you think it's fine I'll leave it, but I suspect I should change.
There was a problem hiding this comment.
Renamed on the presumption (based on above reasoning) that it is appropriate to do so. Let me know if not.
The added toplogies are all topologies that are not being tested before: * compute2 depending directly on inputs (instead of on compute1). Note that this also tests having multiple inputs, something that was not tested before. * compute1 depending on other compute1 for value. Note that the multiple dep changes test has a compute1 depending on compute1... but it only tests for the callback of a dependent compute2 and doesn't check the value of the dependent compute1, so this test is still new. * compute2 depending on other compute2. Why should we care? It is possible that the limited range of topologies in the existing tests will drive some solutions to work only for those specific topologies. Before this commit, the minimal non-generic solution that would pass tests is "when an input is updated, update compute1 cells depending on that input, update compute2 cells depending on those compute1 cells". Again note that the multiple dep changes test has a compute1 -> compute1 connection but the value is never checked, so the test still passes despite this minimal solution not updating it. Knowing this, it could be somewhat tempting to implement such a solution instead of the generic solution. I believe the new tests make clearer that a solution should be generic across all possible topologies. Clearly we can never *ensure* that with just tests, but I think this gets us closer than we were before. Solutions will be more likely to take it into account, and reviewers are freed from having to explain about topology limitations and can focus attention on other areas. Test version is bumped for this.
| // Compute cells can depend on other compute cells. | ||
| func TestTopology(t *testing.T) { | ||
| // Compute 2 cells can depend on compute 1 cells. | ||
| func TestCompute2Diamond(t *testing.T) { |
There was a problem hiding this comment.
renamed for #188 (comment)
|
I think the added granularity here is a huge improvement. @soniakeys do you see any gotchas with this? |
|
I can't comment right away because I hadn't looked at this exercise before. It looks rather complex. I should probably try to solve it on my own first. |
|
Mm, okay, I coded a solution but it doesn't pass the last test, The readme, from x-common/react.md, asks for callback mechanism which would be adequate to propagate values, except this last test disallows it. I'm not sure what the reasoning was there. |
|
[EDIT: Ah, I see, you're asking whether callbacks could be used internally to propagate values. That could be possible. After all, the internal implementation can be anything that meets the interface requirements. However it is also the case that the callbacks are part of the exported interface, which means external users can register callbacks. As such, the semantics of the callbacks need to make sense for external users. See below.] That test was long before my time so you'd have to ask the original author for the very best answer, but here is my attempt at a reasoning for that answer. I agree with the presence of that test. Consider the diamond topology. There's one input. There are two Compute1 cells that depend on that input, let's say they're Let's say initially the value of So the question becomes: Should we fire the callback on the intermediate value of -5? I argue: "No, we should not". The value of -5 only appears while the system is in a half-updated and inconsistent state, thus it doesn't truly make sense to notify the callback of that half-updated value. We should only notify the callback of the final answer once all values have propagated, which is 15. If someone uses this react system to model a circuit, then sure, propagation delays probably mean that the updated signals arrive at ever-so-slightly different times (by necessity of physical considerations), so the value of that multiplier would be in that half-updated state for a fleeting moment, but again I argue that that is not really a value of interest; we really care about the stable values. That's why I think the multiple dependency test is there. If you like I can open a PR to add a comment explaining that. It could be useful to explain it. [EDIT: I think given the above considerations, if callbacks are used internally to implement propagation, any of those internal callbacks probably have to be treated differently than externally-registered callbacks. The external users should not see intermediate values like the -5 in the above example. It would be incredibly surprising behavior and leaks internal implementation details.] |
|
Well, my issues are unrelated to this PR. It looks fine to me and I don't want to hold it up. |
Two commits, the first a small refactor (but does add a small bit of extra functionality) and the second adding new tests. Much more detail provided in the individual commit messages.