-
Notifications
You must be signed in to change notification settings - Fork 1k
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
TestJournal and TestSnaphostStore #3881
TestJournal and TestSnaphostStore #3881
Conversation
Build failed due to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is a very early preview and you have more iterations you're working on, but I wanted to pass along some comments.
src/core/Akka.Persistence.TestKit.Tests/Akka.Persistence.TestKit.Tests.csproj
Show resolved
Hide resolved
d0aa37f
to
f640b69
Compare
f640b69
to
46c77c4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far - I think we should add a new base class on top of the TestKitBase
that makes it more concise to use some of these Journal failure methods when writing a test, The underlying TestJournal
itself looks great though.
@Aaronontheweb I have implemented |
One test in a totally different project (which was not modified) is failing. No idea why. |
@valdisz we have a number of test failures that are due to concurrent tests with hard deadlines blocking threads, We have an open proposal to fix this by moving everything to an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have some more comments I want to add, but I need to do some work with this code locally on my machine first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some examples of the type of functionality I'd like to see built into the PersistentTestkit
class, and some recommendations on moving away from blocking threads and towards async
/ await
implementations
@valdisz this is looking good - what are next steps for implementing this? More overloads on the |
@Aaronontheweb more unit tests for existing code (some are still needed), then SnaphsotStore and then XML DOC. |
7ed6cef
to
aceb9aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great - have some minor changes we need to address (left comments for them) but otherwise I think this is ready to go.
We'll work on getting documentation added to the website for this built into a separate pull request and we'll also plan on releasing this as part of the v1.4.0-beta2 launch, so users can start using it and giving us some feedback. We'll probably consume it inside the Akka.Persistence.Extras project as a starting point.
src/core/Akka.Persistence.TestKit.Xunit2/Akka.Persistence.TestKit.Xunit2.csproj
Outdated
Show resolved
Hide resolved
src/core/Akka.Persistence.TestKit.Xunit2/Akka.Persistence.TestKit.Xunit2.csproj
Show resolved
Hide resolved
/// This class represents an Akka.NET Persistence TestKit that uses <a href="https://xunit.github.io/">xUnit</a> | ||
/// as its testing framework. | ||
/// </summary> | ||
public abstract class PersistenceTestKit : PersistenceTestKitBase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to add a constructor overload here which takes an ITestOutputHelper
for XUnit - that's how we're able to inject the XUnit output capture system into the Akka.NET logging infrastructure. Here's an example of a unit test where a user would do that:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To correctly handle ITestOutputHelper
in test kit I changed the base class of PersistenceTestKit
to TestKit
. This means that PersistenceTestKitBase
no longer exists other test frameworks will not have Test Kit class with persistence testing API out of the box for now.
src/core/Akka.Persistence.TestKit/Akka.Persistence.TestKit.csproj
Outdated
Show resolved
Hide resolved
PR is ready, all work is done. If any issues are found, please comment, otherwise, merge. |
We got some errors in Tests, in the MNTK tests related to persistence are failing with casting errors. However as far as i can tell that should not be code this PR changed. Did we change an IList to an SortedSet somewhere in this PR ? |
This PR does not change anything except adding 3 new projects to the solution. |
Yeah I could not find it either. Hence my question. Just checking if I'm
not crazy. So this means a PR was merged which caused problems. Odd that ci
didn't catch it. I got some time tomorrow ill see if I can find the
offending PR. That is if no one else beats me to it.
Op zo 25 aug. 2019 22:09 schreef Valdis Zobēla <notifications@github.com>:
… We got some errors in Tests, in the MNTK tests related to persistence are
failing with casting errors. However as far as i can tell that should not
be code this PR changed.
Did we change an IList to an SortedSet somewhere in this PR ?
This PR does not change anything except adding 3 new projects to the
solution.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#3881?email_source=notifications&email_token=AAHQ3F5ASNYCGAVCHTJBCVLQGLRGXA5CNFSM4IKM2MFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5C2Y4A#issuecomment-524659824>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAHQ3FZXBLLNYTZ5UJHY62TQGLRGXANCNFSM4IKM2MFA>
.
|
Think i found the issue. Problem happens during json serialization/deserialization. Im thinking this PR is the culprit #3671 Because i cannot find anything else that might cause this. |
Surprisingly my next PR does not fail any builds. #3889 |
That is because we have a system. Which detects what code is affected and
then only builds and runs the projects with the affected code. Cuts back on
ci time. Since your other PR is a docs only PR. Only the docs project is
checked.
Op ma 26 aug. 2019 12:50 schreef Valdis Zobēla <notifications@github.com>:
… Surprisingly my next PR does not fail any builds.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#3881?email_source=notifications&email_token=AAHQ3FZFPJNCGWHQOPUN6ZTQGOYOBA5CNFSM4IKM2MFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5EAPOQ#issuecomment-524814266>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAHQ3F2DIYYXSQ7XM4JYU43QGOYOBANCNFSM4IKM2MFA>
.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I'll take a look at that failing spec - I think it's due to the JSON.NET upgrade. CI didn't run because I think the |
Having a persistent failure with MNTR specs on .NET Framework only:
Going to take a closer look at this - don't think it's related to these changes. |
* TestJournal with Write interception and various failure strategies * Akka.Persistence.TestKit implementation
PR addresses issue #3877
Added 3 new projects:
Akka.Persistence.TestKit
Akka.Persistence.TestKit.Tests
Akka.Persistence.TestKit.Xunit2
It adds a new journal (
TestJournal
) and a new snapshot store (TestSnaphostStore
). It is possible to intercept calls to them and apply user-defined behavior, i.e. to simulate network problems or anything else.It adds new test base class
PersistenceTestKit
(Xunit2 based) to simplify usage. API loos like this: