Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

react: Update tests to v2.0.0 #1346

Merged
merged 8 commits into from
Apr 20, 2018
Merged

Conversation

mrcfps
Copy link

@mrcfps mrcfps commented Feb 22, 2018

Copy link
Contributor

@cmccandless cmccandless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this example solution fails a new test I've suggested at exercism/problem-specifications#1194. Here is an implementation of the test for your convenience.

input_.value = 3
self.assertEqual(callback1.values, [4])

def test_callback_cells_only_fire_on_change(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably be test_callbacks_only_fire_on_change

input_.value = 2
self.assertEqual(callback1.values, [])
input_.value = 4
self.assertEqual(callback1.values, [222])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been thinking about this: I'm not sure that this is the best interpretation of the canonical data, particularly regarding callbacks. As @petertseng pointed out, callbacks should just be executable code; they shouldn't "remember" anything.

I'll have to spend some time thinking about a better way to do this. If you can come up with something first, feel free to refactor and update this PR.

@mrcfps
Copy link
Author

mrcfps commented Feb 24, 2018

@cmccandless My apologies for this weird implementation of Callback. Actually callbacks in the real world are notoriously difficult to test and debug (Node.js developers should have the same feeling). I won't take action until canonical data makes this clear.

mRcfps added 3 commits February 28, 2018 10:31
* `copy()` method to `[:]`
* `clear()` method to reassigning an empty list
@mrcfps
Copy link
Author

mrcfps commented Feb 28, 2018

There are still two open PRs, namely exercism/problem-specifications#1196 and exercism/problem-specifications#1197, so I'm not sure what is the stable version of canonical data. Maybe in this PR updating to v1.2.0 is enough, and we can open another PR to handle later updates.

@cmccandless
Copy link
Contributor

I'm still not sure about this implementation for callbacks... If at all possible, I think I would like to get back to callbacks being essentially just a function (or method).


output.add_callback(callback1)
input_.value = 3
self.assertEqual(callback1.values, [4])
self.assertEqual(output.expect_callback_values(callback1), [4])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that I like expect_callback_values being a member of ComputeCell...

What do you think about something like this?

callback_buffer = []
output.add_callback(callback_buffer.append)
input_.value = 3
self.assertEqual(callback_buffer, [4])

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean convert callback to callback_buffer, and keep outputs of callbacks in the buffer? If so, I guess problem specification of this exercise should be changed, too.

My justification for expect_callback_values being a member of ComputeCell is that ComputeCell seems to be acting as a host for callbacks to be attached to and detached from. Or you can imagine callbacks as plugins for the ComputeCell, so trying to obtain output values of a callback is like reading the state of a plugin.

However, these are just analogies. Insights are welcome.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are discussions on rewording the canonical-data, but I don't think that semantic changes are necessary to it.

Or you can imagine callbacks as plugins for the ComputeCell, so trying to obtain output values of a callback is like reading the state of a plugin.

Close: this is more of a publisher/subscriber scenario. The ComputerCell "publishes" new values by calling the registered callbacks. The callback is the subscription mechanism. The publisher doesn't care what the subscriber object is or how it uses the published data, and has no access to it. In a practical use case, if one were trying to use the solution for this exercise to perform calculations, they might simply use output.add_callback(lambda v: print(v))

I don't remember if I linked @petertseng's comment before (exercism/problem-specifications#1194 (comment)), but it's a good in-context note on what a callback is/should be.

@stale
Copy link

stale bot commented Mar 27, 2018

This issue has been automatically marked as abandoned because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the abandoned label Mar 27, 2018
@stale stale bot removed the abandoned label Mar 27, 2018
@mrcfps
Copy link
Author

mrcfps commented Apr 14, 2018

@cmccandless I've made up my mind to update this exercise to canonical v2.0.0 as soon as I can (sorry for the long delay). Here are two ways to implement unit tests related to callbacks (just take test_compute_cells_fire_callbacks for example):

  • Your suggested way from above:
def test_compute_cells_fire_callbacks(self):
    input_ = InputCell(1)
    output = ComputeCell([input_], lambda inputs: inputs[0] + 1)
    callback_buffer = []

    output.add_callback(callback_buffer.append)
    input_.value = 3
    self.assertEqual(callback_buffer, [4])

But it seems that expect_callbacks operation in canonical data expects only one value for each callback, not an array or a list:

{
  "type": "set_value",
  "cell": "input",
  "value": 3,
  "expect_callbacks": {
    "callback1": 4
  }
}
  • Mine (hack but more intuitive)
from functools import partial

def test_compute_cells_fire_callbacks(self):
    input_ = InputCell(1)
    output = ComputeCell([input_], lambda inputs: inputs[0] + 1)
    observer = float('inf')

    def callback1(observer, value):
        observer = value  # noqa

    output.add_callback(partial(callback1, observer))
    input_.value = 3
    self.assertEqual(observer, 4)

The reason I resort to partial magic here is that observer within test_compute_cells_fire_callbacks cannot be accessed in callback1 (thanks to Python's weird lexical scope), so we have to pass observer as a parameter. Maybe I'm detouring too much? Clever improvements are welcome.

mrcfps and others added 2 commits April 20, 2018 21:38
pig-latin: improve documentation (exercism#1352)

add "Running the tests" section to README template (exercism#1271)

* add "Running the tests" section to README template

* regenerate README files with new template

* rewrite Running the tests section for clarity

* -switch to a plaintext-readable format
-add link explaining py.test vs pytest

queen-attack: re-implement according to canonical data 2.1.0 (exercism#1351)

* queen-attack: re-implement according to canonical data 2.1.0

* queen-attack: rewrite to pass tests v2.1.0

* queen-attack: remove redundant tests

Rm `test_invalid_position_can_attack` since `Queen()`

grains: update tests to v1.1.0 (exercism#1357)

Fix typo and minor style issues in all READMEs (exercism#1356)

* Fix typo and style in README template

* Fix `shold` to `should`
* Fix minor style issues in `### Common pytest options`
* Add blank line after all headers

* Regenerate all READMEs

* Remove redundant periods in READMEs

add `awaiting review` exempt label to stale.yml (exercism#1364)

go-counting: adapt tests to canonical data v1.0.0 (exercism#1360)

* set correct canonical data version

* adapt tests and example solution to canonical data 1.0.0

* use assertSetEqual() consistently

sgf-parsing: implement exercise (exercism#1359)

* implement sgf-parsing

* fix import statement

* create entry in config.json

* fix __eq__ for Python2

lens-person: forego exercise (exercism#1299)

* lens-person: forego exercise

`lens-person` is specific to languages with immutable data (i.e. Haskell). This concept does not exist in Python, so the exercise should be foregone.

* remove bad comma

Implement exercise bank-account (exercism#1260)

* Implement exercise bank-account

* bank-account: generate README using configlet

* bank-account: fix typo and comments

pascals-triangle: update tests to canonical-date v1.2.0 (exercism#1164)

house: return singleton list for single verse (exercism#1354)

* house: return singleton list for single verse

RE exercism#1347 ([comment](exercism#1347 (comment)))

* house: fix example solution to pass changed tests

meetup: remove fail-safe for undefined MeetupDayException (exercism#1345)

* meetup: remove fail-safe for undefined MeetupDayException

Fixes exercism#1344

* meetup: define MeetupDayException in example solution

* meetup: fix example solution to raise correct exception

* meetup: fix flake8 violations

* meetup: add exception message

Curriculum (exercism#1355)

* select core exercises and set exercise ordering

* add missing obvious topics

* make list-ops a core exercise

* rational-numbers: increase difficulty

* unlocked_by core exercises only (exercism/configlet#61)
Ref: exercism/DEPRECATED.v2-feedback#61 (comment)

alphametics: mark computationally intensive test as extra-credit (exercism#1358)

* alphametics: mark computationally intensive test as extra-credit

While this test is canonical, it does not technically add additional coverage. This test serves as a test for efficiency (exercism/problem-specifications#1024 (comment)) of a solution, not completeness.

Furthermore, here are the run-times for this exercise from the [latest Travis build]() (at the time of this writing):
| Python Version | Run-time (seconds) |
| --- | --- |
| 2.7 | 3.155 |
| 3.3 | 2.461 |
| 3.4 | 3.567 |
| 3.5 | 7.270 |
| 3.6 | 0.774 |

Notice that the optimized example solution is only "fast" in 3.6.

* alphametics: add to list of exercises allowed to skip tests in CI

bank-account: update README using configlet (exercism#1366)

go-counting: update README to latest description (exercism#1367)

bracket-push: update tests to v1.3.0 (exercism#1369)

isbn-verifier: update tests to v2.4.0 (exercism#1373)

* Replace test case - "invalid character in isbn"
* Add test case with only 9 digits

Python "bowling" test issue. (exercism#1372)

Fixes /exercism/python/exercism#1371.

yacht: implement exercise (exercism#1368)

* yacht: implement exercise

* yacht: use enumeration of score categories

* Use enumeration instead of plain strings to represent categories
* Improve func `ns` in example solution

crypto-square: Clarify rectangular output requirement in README (exercism#1375)

scale-generator: clarify docs. (exercism#1374)

* Removed most mentions of terms that were irrelevant ("diminished interval") or undefined ("accidentals").
* Removed irrelevant table
* Some light reformatting
@mrcfps mrcfps changed the title react: re-implement according to canonical data react: Update tests to v2.0.0 Apr 20, 2018
Copy link
Contributor

@cmccandless cmccandless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the use of callback_factory and functools.partial! Good changes.

@cmccandless cmccandless merged commit 4478caf into exercism:master Apr 20, 2018
@cmccandless
Copy link
Contributor

@mrcfps merged; thanks for all your work on this PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants