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

Feature: Extend Universal attributes to support async aspects #230

Open
fcastells opened this issue May 2, 2024 · 4 comments
Open

Feature: Extend Universal attributes to support async aspects #230

fcastells opened this issue May 2, 2024 · 4 comments

Comments

@fcastells
Copy link

Describe the solution you'd like
Add the ability to execute async code before, after and on exception in Universal attributes

Additional context
Currently we can apply a Universal attribute to an async method (most generically, a method that returns Task), but it's not possible to run async code around that method.

@pamidur
Copy link
Owner

pamidur commented May 8, 2024

Hi, I've seen your PR. It passes all the tests, but before I merge it. Could you please give me an example on how do you like to use those methods?

I'm just not sure if having two set of methods there is justified. Maybe one set of methods (async only) is enough.

@fcastells
Copy link
Author

Hi, I'm already using this on my project actually (by copying the code for now). My use case is as follows: I have a test suite using Playwright for front-end testing. Each test is divided into steps. What I wanted is to automatically take a screenshot after each step, so I created an aspect that implements OnAfter and OnException, gets the instance, gets the page object from it and then needs to call await page.ScreenshotAsync(). Initially, I did page.ScreenshotAsync().GetAwaiter().GetResult(), but that's not the right way of doing it.

I agree with you that having only the async versions is the right way, but I didn't want to make a breaking change. This is up to you, but maybe you could mark the synchronous versions as obsolete and remove them in a future update, to give time to people to migrate the code.

@pamidur
Copy link
Owner

pamidur commented May 8, 2024

Thank you for clarification. I'll need yet another day to digest this PR.

@fcastells
Copy link
Author

No worries. Let me know if you need further clarification or changes.

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

No branches or pull requests

2 participants