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

OSOE-65: Capture screenshot after each page load #121

Merged
merged 9 commits into from
Feb 3, 2022
Merged

Conversation

Piedone
Copy link
Member

@Piedone Piedone commented Feb 2, 2022

OSOE-65

Fixes #120

@sarahelsaig sarahelsaig merged commit d396924 into dev Feb 3, 2022
Comment on lines +107 to +111
// For some reason doing await afterPageChange.Invoke(this) will case exceptions to not propagate
// when there are more than 2 subscribers.
await afterPageChange.GetInvocationList()
.Cast<PageChangeEventHandler>()
.AwaitEachAsync(eventHandler => eventHandler(this));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why this was necessary, BTW, @DAud-IcI?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand it's not actually safe to call Invoke on a multicast delegate and should be avoided. What multicast does, is calling each function in sequence and returning the last one's value. If you think about delegate in a sync context it just has methods that return a certain type so if you call Invoke synchronously all it does is start three threads and returns you the last one's Task object which the await correctly awaits. So all but the last one are essentially fire-and-forget event calls.

Here is a little experiment to demonstrate the problem:
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we make an InvokeAsync extension method then AsnycFixer should complain about using Invoke. Do you want to do one or both of the following?

  1. Expand scope here and make such an exception or create an issue to tackle it later.
  2. Open an issue at AsyncFixer that it shouldn't allow Invoke on Task-returning delegates (at least if += is ever used on them).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that makes sense. I actually looked into what multicast delegates do and couldn't figure out the reason - in my understanding, async support is available but it's definite that what actually happens is what you said.

Perhaps we need to do something similar for other events in UITestExecutionEvents too. Though the others are actually used in a sync way:

context.Configuration.Events.BeforeClick?.Invoke(context, element).GetAwaiter().GetResult();

Hmm, AsyncFixer didn't complain here, what do you mean? I'd perhaps only add an MulticastDelegate extension to do what we have above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, if you have a SomeType.Method() method and create a MethodAsync(this SomeType sometype) extension method then AsyncFixer will give you error VSTHRD103: condition synchronously blocks. Await MethodAsync instead whenever a someType.Method() call happens.

So if we'd create an InvokeAsync extension for Task returning delegates that would effectively deprecate the regular Invoke method. I realize this is not trivial, becuase you are using a custom delegate and not just Task<T1,Task> so this probably deserves its own issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. Could you please create one here? https://github.com/Lombiq/Helpful-Libraries/issues

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

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.

Capture screenshot after each page load
2 participants