-
Notifications
You must be signed in to change notification settings - Fork 31
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
Add support to yield assertions #275
Conversation
Additional to yielding a condition() function, which returns True or False, this commit allows yielding an assertion method, which returns None on success, but raises AssertionError otherwise. AssertionError is captured and suppressed until timeout is exceeded. condition() returning None is treated as success. Any other return value raises a TypeError. Note: This may break test cases lazily returning non-boolean results.
Well, this seems to be a breaking change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I'm happy you decided to accept my proposal. 2 remarks:
- I'd use
time.monotonic()
instead oftime.time()
, just in case the system clock changes - I'd introduce a (configurable?) sleep interval in case of failure, in order to avoid 100% CPU usage. Such interval may be made incremental, so that it starts "fast" and slowly increases. Something like:
def call_until(assert_call, timeout=5, interval=0.0001):
stop_at = time.monotonic() + timeout
while True:
try:
return assert_call()
except AssertionError:
if time.monotonic() > stop_at:
raise TimeoutError
# Progressive interval:
# 0.0001, 0.0002, 0.0004, 0.0008, 0.0016, 0.0032, 0.0064,
# 0.0128, 0.02, 0.02, 0.02, 0.02, ...
interval = min(interval * 2, 0.02)
time.sleep(interval)
But perhaps this is already handled internally, in which case please ignore my remark.
The runner shouldn't consume much CPU while waiting as it works nearly like an asyncio loop. It schedules every cycle/call via sublime.set_timeout() using a predefined poll interval (17ms). To configure polling interval, you'd need to use the dictionary approach, passing an assertion as callable. A global timeout can be specified via config. Otherwice via dictionary approach as well. Arleady thinking of using a class instead. Theoretically it would currently be possible to |
Perfect. Thanks.
I'm also having second thoughts about my original proposal, that was: def test_something(self):
self.window.run_command("show_panel", {"panel": "console"})
yield lambda: self.assertEqual(self.window.active_panel(), "console") I now realize this is inconsistent, because it breaks the promise that code will block until the lambda evaluates to def test_something(self):
self.window.run_command("show_panel", {"panel": "console"})
yield from self.assertEqual(self.window.active_panel(), "console") ...but I'm not entirely sure if it's possible to implement (a function that works both as a normal callable AND a generator). I'm probably biased by the fact that in my own unit tests I have tons of idioms that look like this: view = yield from self.await_open_file(fname)
yield from self.await_close_view(view)
yield from self.await_close_window(window)
yield from self.await_content_in_view(view, expected)
yield from self.await_close_input_panel()
yield from self.await_clear_view(view) This would basically follow the same approach. What do you think? |
The point of |
I don't think that is correct as we want to return the value of the getter to the test function. Maybe we await a view, a panel, or a find_by_selector-region. E.g.
Generally
iff I think the documentation is wrong
It is a truthy/falsy check and the runner will not wait but re-try of course. It is difficult to make this work as the idea behind the condition was that it is a getter, but During development you could put print statements inside the lambda. Or we can introduce a special form
Basically a tuple of the form Or we wrap all |
IMHO, the tuple form would just be an alternative to the existing I am also not convinced of re-eventing or wrapping each method of unittest's base classes. This package just provides a runner for unittest which runs within ST plugin context. This PR just exists because #274 seemed not to satisfy the requestor of #273. The different natures of yielding getters to sync test runs vs. assertions has already been discussed in the initial issue. With regards to possible backward compatibility implications, the whole thing seems rather like a dead end (as I already expected). |
Hm, my tuple thing is just non-sense. I can imagine doing
Even with a trivial tagging function
we can discriminate using I can imagine that such a ADD: def assertE(a, b):
rv = a == b
if not rv:
return fail(f"{a} != {b}")
return rv |
Followup to (really) resolve #273
Additional to yielding a condition() function, which returns True or False, this commit allows yielding an assertion method, which returns None on success, but raises AssertionError otherwise.
AssertionError is captured and suppressed until timeout is exceeded.
condition() returning None is treated as success.
Any other return value raises a TypeError.
Note: This may break test cases lazily returning non-boolean results.