Skip to content

Conversation

@kaste
Copy link
Contributor

@kaste kaste commented Dec 1, 2018

  1. Currently, a yield <conditon> where the condition itself throws leaves the test in an undefined state.

E.g. yield panel.find(...) when panel is None

This PR reraises the error inside the generator (gen.throw()).

  1. It is also useful and cheap to implement to get the last value back in the test. This is implemented using gen.send() instead of just next(gen)
        region = yield from lambda: panel.find(COMMIT_1, 0, sublime.LITERAL)
		# assert that region is placed correctly
  1. From a tester standpoint a condition that times out must throw, currently it 'continues anyway'.

E.g. in the GitSavvy tests I often have to do

        yield from lambda: panel.find(COMMIT_1, 0, sublime.LITERAL)

        # `yield condition` will continue after a timeout so we need
        # to actually assert here
        actual = panel.find(COMMIT_1, 0, sublime.LITERAL)
        self.assertTrue(actual)

@kaste
Copy link
Contributor Author

kaste commented Dec 1, 2018

Okay, seems that we have test to ensure the old (3) functionality. Wonder what the rationale here is. I assumed the original author didn't know how to raise inside the generator, so that 'continue anyway' was their only option. Anyway, the first commit handles (1) and (2) and can be cherry picked. (I'm also pretty sure that my solution to timeouts (3) is the preferable behavior.)

@Thom1729
Copy link
Member

Thom1729 commented Dec 2, 2018

I've been thinking of factoring out the deferred logic from UnitTesting and stealing it for sublime_lib, and I agree that this is probably the right approach.

@kaste
Copy link
Contributor Author

kaste commented Dec 5, 2018

Here I think we should merge this although it is technically breaking behavior. But we log a good error message.

I actually have failing conditions but passing tests while writing tests, and I think it tells me that I await the wrong thing. (Or assert the wrong thing LOL)

@randy3k WDYT?

@kaste
Copy link
Contributor Author

kaste commented Dec 6, 2018

Breaking is breaking and I will not push it as a 'fix/patch'. I would cherry-pick (1) and (2) and then implement (3) the breaking bit together with #146. That means (3) is then available behind a flag since we don't have sem versioning with PackageControl.

@kaste kaste mentioned this pull request Dec 10, 2018
@randy3k randy3k closed this in #150 Dec 20, 2018
@kaste kaste deleted the better_yield_condition branch December 20, 2018 10:33
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.

2 participants