Skip to content

Task func extensions #1011

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.

Sign up for GitHub

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

Jump to bottom
Closed
lg2de wants to merge 7 commits into from
Closed

Task func extensions #1011

lg2de wants to merge 7 commits into from

Conversation

@lg2de
Copy link
Contributor

@lg2de 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
Copy link
Member

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
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 lg2de mentioned this pull request Mar 23, 2019
Closed
@lg2de
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.

@lg2de lg2de mentioned this pull request May 27, 2019
Merged
@lg2de
Copy link
Contributor Author

lg2de commented Sep 13, 2019

abort

@lg2de lg2de closed this Sep 13, 2019
@lg2de lg2de deleted the TaskFuncExtensions branch March 8, 2020 12:31
@jnyrup jnyrup mentioned this pull request Apr 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

future

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lg2de @jnyrup @dennisdoomen