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

Avoid NullReferenceException with async methods #66

Merged
merged 2 commits into from
Dec 13, 2013

Conversation

alextercete
Copy link

When using async methods and loose Mock Behavior, calls to Task methods give NullReferenceException, since default(Task) is null. Calls to Task.Wait() and Task<T>.Result should follow the loose behavior: do nothing and return the default value for T, respectively.

Related to #64.

@kzu
Copy link
Contributor

kzu commented Dec 11, 2013

Moq should be .NET 3.5 compatible.

@alextercete
Copy link
Author

I think I missed that information, since Moq.csproj has .NET 4.0 target framework.

Since Task is only available in .NET 4.0 or greater, how do you suggest we address this?

@kzu
Copy link
Contributor

kzu commented Dec 12, 2013

This is the script that builds .net35 and 40 of Moq.

Conditional directives should be used to add .NET4.0+ features, and as long as you can run that script and get a valid Moq.nupkg output, we're good :)

@alextercete alextercete reopened this Dec 12, 2013
@alextercete
Copy link
Author

Sorry, I accidentally closed the Pull Request. @danielkzu, can you check if there is anything missing to merge this?

@kzu
Copy link
Contributor

kzu commented Dec 13, 2013

Alex, I see a lot of files changed in the pull request that actually haven't changed. Looks like whitespace changes only.

Could you clean that up so that the request includes only what actually changed? That would be substantially easier to review and merge.

Thanks in advance and thanks for putting in the effort to fix this issue!

@alextercete
Copy link
Author

Hey Daniel, these so called "whitespace changes" are actually intentional. You've created a while ago the file .gitattributes with * text=auto which means to convert every CRLF line-ending to LF before saving files to Git. However, this only applies to files added to Git after you've commited .gitattributes. It means that files added before that happened will still have CRLF line-endings until you re-normalize the repository.

For anyone using Git in Windows, a lot of modified files will appear just after a clean clone. That happens because since the file has CRLF line-endings in the repo and you're telling Git to change all CRLF to LF, the diff shows only line-ending changes (which are impossible to get rid off, by the way)

I've seen other contributors having trouble with that recently, so I think it is worth taking a look at. I tried to interactively rebase my work to remove the repository re-normalizing commit, but then I cannot apply my changes due to local changes.

If you decide to do this fix directly to the master branch, I'll happily reapply my work on top of it.

@kzu
Copy link
Contributor

kzu commented Dec 13, 2013

Good to know!

Would you mind doing the forced re-normalization of the entire repo and
sending that as a separate PR? After that, merging the new one should be a
breeze.

Thanks!

/kzu

Daniel Cazzulino

On Fri, Dec 13, 2013 at 9:10 AM, Alex Tercete notifications@github.comwrote:

Hey Daniel, these so called "whitespace changes" are actually intentional.
You've created a while ago the file .gitattributes with * text=auto which
means to convert every CRLF line-ending to LF before saving files to Git.
However, this only applies to files added to Git after you've commited
.gitattributes. It means that files added before that happened will
still have CRLF line-endings until you re-normalize the repositoryhttps://help.github.com/articles/dealing-with-line-endings#re-normalizing-a-repository.

For anyone using Git in Windows, a lot of modified files will appear just
after a clean clone. That happens because since the file has CRLF
line-endings in the repo and you're telling Git to change all CRLF to LF,
the diff shows only line-ending changes (which are impossible to get rid
off, by the way)

I've seen other contributorshttps://github.com//pull/63#issuecomment-30473148having trouble with that recently, so I think it is worth taking a look at.
I tried to interactively rebase my work to remove the repository
re-normalizing commit, but then I cannot apply my changes due to local
changes.

If you decide to do this fix directly to the master branch, I'll happily
reapply my work on top of it.


Reply to this email directly or view it on GitHubhttps://github.com//pull/66#issuecomment-30505067
.

Alex Tercete added 2 commits December 13, 2013 13:40
When using async methods and loose Mock Behavior, calls to Task methods
give NullReferenceException, since default(Task) is null. Calls to
Task.Wait() and Task<T>.Result should follow the loose behavior: do
nothing and return the default value for T, respectively.

Related to devlooped#64.
Use compile directives to ensure smooth NuGet package generation.

'IReturnsExtensions' was using 'System.Threading.Tasks' namespace, so
compile directives were also added to it.
@alextercete
Copy link
Author

There you go, much cleaner indeed! ;)

@@ -1,4 +1,5 @@
using System;
#if !NET3x
using System;
Copy link
Contributor

Choose a reason for hiding this comment

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

Dumb me. The entire file is not available on NET3x :)

kzu added a commit that referenced this pull request Dec 13, 2013
Avoid NullReferenceException with async methods
@kzu kzu merged commit b24199e into devlooped:master Dec 13, 2013
@kzu
Copy link
Contributor

kzu commented Dec 13, 2013

Mmm... the build is broken for the Silverlight project:

d:\temp\tmpE930\Source\EmptyDefaultValueProvider.cs(47,24): error
CS0234: The type or namespace name 'Tasks' does not exist in the
namespace 'System.Threading' (are you missing an assembly reference?)
[D:\temp\tmpE930\Source.Silverlight\Moq.Silverlight.csproj]

As soon as the build is fixed, I'll ship this as 4.2.

Did you try running build.bat on your end?

/kzu

Daniel Cazzulino

On Fri, Dec 13, 2013 at 12:43 PM, Alex Tercete notifications@github.comwrote:

There you go, much cleaner indeed! ;)


Reply to this email directly or view it on GitHubhttps://github.com//pull/66#issuecomment-30518550
.

@alextercete
Copy link
Author

I did run build.cmd, but since I didn't have Silverlight installed on my machine, it gave me an error. I did a quick Google search and found out that System.Threading.Tasks was supported in Silverlight, so I decided it was good enough. I guess I should have been more careful.

Are doing the fix or do you want me to do it instead?

@kzu
Copy link
Contributor

kzu commented Dec 13, 2013

Swamped at the moment in other things. Could you take a look?

Thanks!

/kzu

Daniel Cazzulino

On Fri, Dec 13, 2013 at 1:07 PM, Alex Tercete notifications@github.comwrote:

I did run build.cmd, but since I didn't have Silverlight installed on my
machine, it gave me an error. I did a quick Google search and found out
that System.Threading.Tasks was supported in Silverlight, so I decided it
was good enough. I guess I should have been more careful.

Are doing the fix or do you want me to do it instead?


Reply to this email directly or view it on GitHubhttps://github.com//pull/66#issuecomment-30520509
.

@iskiselev
Copy link
Contributor

TPL presents in Silverlight 5. So, if Moq will provide separate SL4 and SL5 version or drop SL4 support, it can use auto async proxy creation for Silverlight.

@kzu
Copy link
Contributor

kzu commented Jan 9, 2015

I'm happy to drop SL4 unless there is a massive outcry to keep it ;)

/kzu from mobile
On Jan 8, 2015 10:52 PM, "Igor Kiselev" notifications@github.com wrote:

TPL presents in Silverlight 5. So, if Moq will provide separate SL4 and
SL5 version or drop SL4 support, it can use auto async proxy creation for
Silverlight.

Reply to this email directly or view it on GitHub
#66 (comment).

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.

None yet

4 participants