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

External ActorSystem for Testkit event filters. #1753

Merged
merged 1 commit into from
Mar 17, 2016
Merged

External ActorSystem for Testkit event filters. #1753

merged 1 commit into from
Mar 17, 2016

Conversation

akoshelev
Copy link

This commit resolves issue described in #1630. EventFilters now can be
created for user-defined actor systems, not only TestKit.Sys.

/// </summary>
/// <param name="system">Actor system.</param>
/// <returns>A new instance of <see cref="EventFilterFactory"/>.</returns>
public EventFilterFactory CreateEventFilter(ActorSystem system)
Copy link
Member

Choose a reason for hiding this comment

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

Looks good - so this means that the default EventFilter will still work exactly as expected then. That was the only major thing I wanted to check first.

@Aaronontheweb
Copy link
Member

The code changes look fine - but could you please add a spec that verifies that the testkit works for a secondary ActorSystem? Want to have a spec for behavior verification against regressions in the future.

@Aaronontheweb
Copy link
Member

ping @akoshelev - did you see my comment above?

This commit resolves issue described in #1630. EventFilters now can be
created for user-defined actor systems, not only TestKit.Sys.
@akoshelev
Copy link
Author

@Aaronontheweb Hi,
sorry I did not have a chance to respond.
It's all done, I've changed existing unit tests to cover this case as well

Aaronontheweb added a commit that referenced this pull request Mar 17, 2016
External ActorSystem for Testkit event filters.
@Aaronontheweb Aaronontheweb merged commit 1f293cd into akkadotnet:dev Mar 17, 2016
@Aaronontheweb
Copy link
Member

thanks @akoshelev - I've reviewed it and I think it looks good. Thanks for your contribution.

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

Successfully merging this pull request may close these issues.

3 participants