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

Task func extensions #1011

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@lg2de
Copy link
Contributor

lg2de commented Mar 19, 2019

This PR wants to implement the issues #990 and #1001 .
Maybe it is not yet completed, but I think it is time for a review.

Please note:

Timing issues

I implemented new functionality which is related to timing. Unfortunately this cause failed tests from time to time because the real timing cannot be setup as exact as required (without creating real slow tests).

I already have seen some other tests which have the same problem. But I have no (easy) idea yet, how to get the problem fixed.

I think it will only work if the timing is fully mocked. We could introduce a new internal interface like that:

internal interface ITimer
{
    Task Delay(TimeSpan delay);
}

Maybe you know already a good abstraction for the timer.
Anyway, the full solution is a new issue, I guess.

Unit test method naming

I added several unit tests for the new functions. But for me, your naming style for the test methods is completely new (never seen before).

I tried to build names according to the existing tests. Hopefully it is ok. Otherwise please tell me the correct names or even better: Please just rename the tests directly, if needed.

BTW: What do you think about the popular style [UnitOfWork_StateUnderTest_ExpectedBehavior]?

@jnyrup

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Mar 19, 2019

Are you aware that #990 is marked with breaking change label?
As we follow semantic versioning, we try to our very best only to introduce breaking changes in major releases.
The CoreFX repo has a nice guide about what counts as a source or binary breaking change.

The master branch should always be deployable, i.e. be ready to package as a nupkg and releasing on nuget.org.
As such the breaking changes in this PR blocks it from being merged to master.

@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Mar 20, 2019

Indeed, @jnyrup is right. And I don't foresee any 6.0 version before the end of the year. Would it be possible to only include the changes needed for #1001?

@lg2de

This comment has been minimized.

Copy link
Contributor Author

lg2de commented Mar 23, 2019

Hi @jnyrup and @dennisdoomen!

Yes, I had seen the label "breaking change". You are absolutely right, the changes are not binary compatible.

But for my understanding, the backend classes like AsyncFunctionAssertions are not intended for direct use. The users of FA will only use the Should() method which returns the correct implementation. The user is not interested in the concrete class.

I found that FA is a strong named assembly. So, changing version to 5.7.X would require recompilation anyway. Do we really need to wait for version 6.0 for this update? What are your planes for 6.0 to wait until end of year?

With PR #1013 I provided subset of this PR for #1001, as requested.

@dennisdoomen dennisdoomen added the future label Apr 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.