ScreenPyHQ / screenpy

Screenplay pattern base for Python automated UI test suites.
MIT License
28 stars 4 forks source link

If the Actor encounters an exception during `cleans_up`, the cleanup queue does not get cleared. #6

Closed pgoy-dod closed 2 years ago

pgoy-dod commented 2 years ago

It's hard to say which is desired behavior here, but in our specific case, it is not desired.

We directly call the_actor.cleans_up() in our teardown step so we can take screenshots after the attempted teardown. If we encounter an error during teardown, the cleanup queue is not cleared, so the Actor attempts to clean up again when they exit.

Maybe put a try/finally around it so the cleanup queue is cleared no matter what happened?

perrygoy commented 2 years ago

We're having some good discussion about this issue on the PR ( #17 ), but to summarize:

So maybe the correct approach is to add some more methods, has_dependent_cleanup_tasks and has_independent_cleanup_tasks, which add to their own queues. Cleanup should still try them both and clear them after the attempt, but one (dependent) should stop at first failure and the other (independent) should try every task in the list.

@bandophahita and @langgaibo, how do you feel about this? And should has_dependent_cleanup_tasks be an alias of has_cleanup_tasks? Should we have that assumption that the cleanup tasks are a sequence? Otherwise we'd be forcing everyone (all, uh, one suite that i know of :P) to change their has_cleanup_tasks calls.

bandophahita commented 2 years ago

Would it be too lazy of us to leave the implementation of tasks that can continue up to the caller? If they want cleanup to continue on certain tasks, they should wrap those tasks in a try/except layer? it keeps the whole question of what task run and how out of the actor.

perrygoy commented 2 years ago

I think it's helpful to have the basic logic in the Actor itself, the way we landed in the PR. The only reason i consider cleanup tricky enough support it in the Actor is because of some cleanup tasks needing the Abilities that are about to be forgotten.

We could have taken a different approach, maybe adding another alias for attempts_to (i have no idea how to make the English work for it, performs_clean_up_by??) and telling folks to add their cleanup Actions to the tests themselves. That would probably be more in line with a typical test-writing experience...