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

Add support to yield assertions #275

Closed
wants to merge 1 commit into from
Closed

Conversation

deathaxe
Copy link
Member

@deathaxe deathaxe commented Jun 3, 2024

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.

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.
@deathaxe
Copy link
Member Author

deathaxe commented Jun 3, 2024

Well, this seems to be a breaking change.

Copy link
Contributor

@giampaolo giampaolo left a 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 of time.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.

@deathaxe
Copy link
Member Author

deathaxe commented Jun 4, 2024

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 my_val = yield getter to wait for results asynchronously. With this PR it wouldn't be possible anymore as I can't see a way how to distinguish a normal function from assertions. This PR is therefore a breaking change, which may or may not affect anyone's test cases. Maybe risk is rather low, if all used it as documented, but I can't ensure that.

@giampaolo
Copy link
Contributor

giampaolo commented Jun 4, 2024

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).

Perfect. Thanks.

With this PR it wouldn't be possible anymore as I can't see a way how to distinguish a normal function from assertions.

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 True. I expect this would create confusion. After sleeping over this, I think a better approach would be:

    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?

@deathaxe
Copy link
Member Author

deathaxe commented Jun 5, 2024

yield from won't work as it hands over control to called method without the runner to be able to intercept this call. Instead the method is called immediately once, which is the same as calling it synchronously.

The point of yield lambda: ... is to return a callable to the runner, so it can decide what to do.

@kaste
Copy link
Contributor

kaste commented Jun 6, 2024

yield callable() must return bool or None

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.

region = yield lambda: view.find(SELECTED_FILE, 0, sublime.LITERAL)

Generally yield from works as expected, e.g.

interface, view = yield from self.await_std_interface()

iff await_std_interface is a generator too.

I think the documentation is wrong

If the result is not None, the runner continues the generator, if not, the runner will wait until the condition is met with the default timeout of 4s.

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 assertEquals is a sink/setter/side-effect.

During development you could put print statements inside the lambda. Or we can introduce a special form

yield self.assertEquals, a, b
region = yield view.find, SELECTED_FILE, 0, sublime.LITERAL

Basically a tuple of the form (fn, *args) so we can inspect the type of the function. Or a partial, because we can inspect that too.

Or we wrap all self.assert* methods to return a truthy value. yield lambda: self.assertEqual(self.window.active_panel(), "console") or True is also simple.

@deathaxe
Copy link
Member Author

deathaxe commented Jun 6, 2024

IMHO, the tuple form would just be an alternative to the existing yield lambda.... I don't think we should use it to distinguish getters from assert... methods.

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).

@kaste
Copy link
Contributor

kaste commented Jun 6, 2024

Hm, my tuple thing is just non-sense.

I can imagine doing

yield lambda: self.window.active_panel() == "console" or fail(f"{self.window.active_panel()} != console")

Even with a trivial tagging function

class fail(str): pass

we can discriminate using isinstance checks. (isinstance(send_value, fail) counts as failure, and if we timeout we take the message and print it.) That is also readable.

I can imagine that such a fail function would allow building custom assert functions. So that
would be in user-land and we would just provide this basic building block.

ADD:

def assertE(a, b):
    rv = a == b
    if not rv:
        return fail(f"{a} != {b}")
    return rv

@deathaxe deathaxe closed this Jul 6, 2024
@deathaxe deathaxe deleted the feat/yield-assertion branch July 6, 2024 12:44
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.

Add support for yield self.assert*(...)
3 participants