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

AsyncStateMachineAttribute should only be checked by its name #64

Closed
MrJul opened this Issue Dec 5, 2013 · 19 comments

Comments

Projects
None yet
4 participants
@MrJul

MrJul commented Dec 5, 2013

I'm using some async tests with NUnit 2.6.3 in a .NET 4.0 project. The projects uses the Microsoft.Bcl.Async NuGet package to enable the usage of async/await keywords without targeting .NET 4.5.

However, NUnit searches for the AsyncStateMachineAttribute using Type.GetType, which looks only in mscorlib: the attribute isn't found since it resides in another assembly. As a result, none of the async methods are detected correctly.

I think that the attribute presence should only be checked by its full name.

@CharliePoole

This comment has been minimized.

Member

CharliePoole commented Dec 5, 2013

Makes sense.
On Dec 5, 2013 5:48 AM, "Julien Lebosquain" notifications@github.com
wrote:

I'm using some async tests with NUnit 2.6.3 in a .NET 4.0 project. The
projects uses the Microsoft.Bcl.Asynchttp://www.nuget.org/packages/Microsoft.Bcl.AsyncNuGet package to enable the usage of
async/await keywords without targeting .NET 4.5.

However, NUnit searches for the AsyncStateMachineAttribute using
Type.GetType, which looks only in mscorlib: the attribute isn't found
since it resides in another assembly. As a result, none of the async
methods are detected correctly.

I think that the attribute presence should only be checked by its full
name.


Reply to this email directly or view it on GitHubhttps://github.com//issues/64
.

@simoneb

This comment has been minimized.

Contributor

simoneb commented Dec 5, 2013

Makes sense for me. I'm just not sure about how to implement it. I didn't have a chance to try it yet and MSDN is unsurprisingly cryptic:

You can use the GetType method to obtain a Type object for a type in another assembly, if the you know its namespace-qualified name. GetType causes loading of the assembly specified in typeName.

Besides the typo, I believe it's not correct that if you know the namespace-qualified name you can load a type from another assembly (assuming that namespace-qualified name means a string in the form namespace.type but without assembly information). Rather, if you want to load a type from an assembly other than the current and mscorlib you need to supply the fully-qualified name, which includes namespace, type and (partial) assembly name. If that's the case, it means that to support Microsoft.Async.Bcl we would need to specify the assembly name as well, and the namespace in case it's different from that of the same attribute in mscorlib, and that doesn't sound like a great idea.

@MrJul

This comment has been minimized.

MrJul commented Dec 5, 2013

Yes, it should probably be "fully qualified assembly name" in that documentation. However if you look at the parameter doc, you can read:

The assembly-qualified name of the type to get. See AssemblyQualifiedName. If the type is in the currently executing assembly or in Mscorlib.dll, it is sufficient to supply the type name qualified by its namespace.

What I'm proposing is not to specify an assembly name, that won't be portable at all, but to avoid using GetType completely. Something along the lines of:

methodInfo
  .GetCustomAttributes(false)
  .Any(attr => attr.GetType().FullName == "System.Runtime.CompilerServices.AsyncStateMachineAttribute")

The attribute can't be in another namespace, that's what the compiler is looking for. The assembly doesn't matter though.

@simoneb

This comment has been minimized.

Contributor

simoneb commented Dec 6, 2013

Yes, it would be a sensible implementation. Less elegant than IsDefined and I believe it would have the side effect of loading the assemblies of all attributes used on the method, but because we're talking about tests we don't really expect them to have many other attributes besides NUnit's own.

@CharliePoole

This comment has been minimized.

Member

CharliePoole commented Dec 6, 2013

That's how NUnit 2.x found all it's own attributes, so we have the code!

We can either use reflection each time we need to check for the attribute
or only the first time, caching the actual type found.

Charlie

On Thu, Dec 5, 2013 at 3:56 PM, Julien Lebosquain
notifications@github.comwrote:

Yes, it should probably be "fully qualified assembly name" in that
documentation. However if you look at the parameter doc, you can read:

The assembly-qualified name of the type to get. See AssemblyQualifiedName.
If the type is in the currently executing assembly or in Mscorlib.dll, it
is sufficient to supply the type name qualified by its namespace.

What I'm proposing is not to specify an assembly name, that won't be
portable at all, but to avoid using GetType completely. Something along
the lines of:

methodInfo
.GetCustomAttributes(false)
.Any(attr => attr.GetType().FullName == "System.Runtime.CompilerServices.AsyncStateMachineAttribute")

The attribute can't be in another namespace, that's what the compiler is
looking for. The assembly doesn't matter though.


Reply to this email directly or view it on GitHubhttps://github.com//issues/64#issuecomment-29951426
.

@jpierson

This comment has been minimized.

jpierson commented Feb 3, 2014

As mentioned in issue #75, another solution may be to just require async tests to return type Task and then use that task to determine failure by registering continuations on that task. I'm not familiar with the NUnit source code but this seems to be the most sensible solution to me since it would allow for any test formulated directly using TPL API or by using the newer async keyword to be understood directly by NUnit.

Just as a note, we do also fall in the category of those using Microsoft.Bcl.Async for async support on .NET Framework 4.0 mainly to keep compatibility with Windows XP. So although the solution proposed here would fix our problem, I'm still more in favor of a general TPL based solution in absence of any clear advantage of any other solution.

@simoneb

This comment has been minimized.

Contributor

simoneb commented Feb 5, 2014

@charlie, I've come across a first blocking issue when trying to write tests for this. It requires some nuget packages to be referenced by test projects only when building for .NET 4.0. I realized that the build scripts at this point are unaware of which framework they're building as differences are handled in the source code with precompiler directives. I'd like to hear your thoughts about how to handle this framework-specific dependency during the build, right now the only thing I can think of is to introduce framework awareness into the build scripts, which doesn't sound too appealing.

@CharliePoole

This comment has been minimized.

Member

CharliePoole commented Feb 5, 2014

Hi Simone,

Of course, the build scripts are not entirely framework-agnostic, since
they set up the precompiler defines. They used to do more. For example,
here's a scrap from the nunitlite.framework.build script...

<include name="Constraints/**/*.cs"/>
<exclude name="Constraints/BinarySerializableConstraint.cs"

if="${runtime.platform == 'silverlight'}"/>



I got rid of this stuff because it's hard to maintain... a new file is
added and we have to remember to update the script.

If we need it, we can go back to doing this sort of thing for references.
We should make sure that the code that actually makes use of the reference
is surrounded by a pre-compiler directive, of course.

Charlie

On Wed, Feb 5, 2014 at 2:00 AM, simoneb notifications@github.com wrote:

@charlie https://github.com/charlie, I've come across a first blocking
issue when trying to write tests for this. It requires some nuget packages
to be referenced by test projects only when building for .NET 4.0. I
realized that the build scripts at this point are unaware of which
framework they're building as differences are handled in the source code
with precompiler directives. I'd like to hear your thoughts about how to
handle this framework-specific dependency during the build, right now the
only thing I can think of is to introduce framework awareness into the
build scripts, which doesn't sound too appealing.

Reply to this email directly or view it on GitHubhttps://github.com//issues/64#issuecomment-34127280
.

@simoneb

This comment has been minimized.

Contributor

simoneb commented Feb 5, 2014

I guess that if we want to test this feature we'll have to do something like that again, because the variation here does not pertain source code but rather assembly references.
Unfortunately I've come across another issue, which makes me rethink whether this is a simple fix or not.
Basically, to test this feature we need to start running the tests for async even when we compile under .NET 4.0. The problem that I discovered when spending several hours trying to make it work is that you cannot use the C# 4 compiler to compile them. The Bcl Async NuGet package only enables runtime support, but you need a version of the compiler which understands async/await.
This seems obvious in retrospect, but I was under the wrong assumption that the NuGet package was doing some magic that would make this work.

Now, NAnt automatically uses the C# 4 compiler when targeting .NET 4.0, obviously, which fails to build such files. In other words we need to use the C# 5 compiler, instructing it to generate an output assembly for .NET 4.0. VS achieves it by generating a temporary C# file with these contents, which gets compiled together with all other source files:

// <autogenerated />
using System;
using System.Reflection;
[assembly: global::System.Runtime.Versioning.TargetFrameworkAttribute(".NETFramework,Version=v4.0", FrameworkDisplayName = ".NET Framework 4")]

So I'm looking for input here. One option would be to add yet another build flavor to our build script which requires .NET 4.5 but targets .NET 4.0. Another option, a more disruptive one compared to our current approach, would be to always use the C# 5 compiler (so always require .NET 4.5 to build NUnit for .NET) and target different versions of the framework using the approach described above, although I'm not sure it can go back to .NET 3.5 as well.

@CharliePoole

This comment has been minimized.

Member

CharliePoole commented Feb 5, 2014

I like the approach of always using the C# 5 compiler, since that's what
happens when we compile under a sufficiently advanced version of VS.

I have actually looked for a way to tell a higher level compiler to target
a lower level framework, like building for .NET 2.0 under C# 4.0, and I
never found this attribute that you have come up with. If so, I think we
can get the build to do it.

Charlie

On Wed, Feb 5, 2014 at 10:18 AM, simoneb notifications@github.com wrote:

I guess that if we want to test this feature we'll have to do something
like that again, because the variation here does not pertain source code
but rather assembly references.
Unfortunately I've come across another issue, which makes me rethink
whether this is a simple fix or not.
Basically, to test this feature we need to start running the tests for
async even when we compile under .NET 4.0. The problem that I discovered
when spending several hours trying to make it work is that you cannot use
the C# 4 compiler to compile them. The Bcl Async NuGet package only enables
runtime support, but you need a version of the compiler which understands
async/await.
This seems obvious in retrospect, but I was under the wrong assumption
that the NuGet package was doing some magic that would make this work.

Now, NAnt automatically uses the C# 4 compiler when targeting .NET 4.0,
obviously, which fails to build such files. In other words we need to use
the C# 5 compiler, instructing it to generate an output assembly for .NET
4.0. VS achieves it by generating a temporary C# file with these contents,
which gets compiled together with all other source files:

// using System;using System.Reflection;[assembly: global::System.Runtime.Versioning.TargetFrameworkAttribute(".NETFramework,Version=v4.0", FrameworkDisplayName = ".NET Framework 4")]

So I'm looking for input here. One option would be to add yet another
build flavor to our build script which requires .NET 4.5 but targets .NET
4.0. Another option, a more disruptive one compared to our current
approach, would be to always use the C# 5 compiler (so always require .NET
4.5 to build NUnit for .NET) and target different versions of the framework
using the approach described above, although I'm not sure it can go back to
.NET 3.5 as well.

Reply to this email directly or view it on GitHubhttps://github.com//issues/64#issuecomment-34149130
.

@CharliePoole CharliePoole added this to the 3.0 milestone Mar 2, 2014

@simoneb

This comment has been minimized.

Contributor

simoneb commented Mar 10, 2014

I'm now going back to this issue after we have (partially) got rid of NAnt and now using only MSBuild and always the latest build tools to compile.

@CharliePoole

This comment has been minimized.

Member

CharliePoole commented Mar 10, 2014

You may want to consider at the same time moving code that allows async
execution into Reflect.cs, so that async methods are automatically invoked
correctly, for whatever purpose we call them.

On Mon, Mar 10, 2014 at 6:09 AM, simoneb notifications@github.com wrote:

I'm now going back to this issue after we have (partially) got rid of NAnt
and now using only MSBuild and always the latest build tools to compile.

Reply to this email directly or view it on GitHubhttps://github.com//issues/64#issuecomment-37179875
.

@simoneb

This comment has been minimized.

Contributor

simoneb commented Mar 10, 2014

In any case we're stuck because it's not feasible to get NuGet to work on the build server right now due to the old versions on mono installed both on Windows and on Linux.

@CharliePoole

This comment has been minimized.

Member

CharliePoole commented Mar 10, 2014

Remind me what we use NuGet for currently.

On Mon, Mar 10, 2014 at 10:50 AM, simoneb notifications@github.com wrote:

In any case we're stuck because it's not feasible to get NuGet to work on
the build server right now due to the old versions on mono installed both
on Windows and on Linux.

Reply to this email directly or view it on GitHubhttps://github.com//issues/64#issuecomment-37212311
.

@simoneb

This comment has been minimized.

Contributor

simoneb commented Mar 10, 2014

Async on .net 4. Not currently, but on the branch of this issue.
On 10 Mar 2014 20:54, "CharliePoole" notifications@github.com wrote:

Remind me what we use NuGet for currently.

On Mon, Mar 10, 2014 at 10:50 AM, simoneb notifications@github.com
wrote:

In any case we're stuck because it's not feasible to get NuGet to work on
the build server right now due to the old versions on mono installed both
on Windows and on Linux.

Reply to this email directly or view it on GitHub<
https://github.com/nunit/nunit-framework/issues/64#issuecomment-37212311>
.

Reply to this email directly or view it on GitHubhttps://github.com//issues/64#issuecomment-37226434
.

@CharliePoole

This comment has been minimized.

Member

CharliePoole commented Mar 10, 2014

I think if we use NuGet, we should be sure to ask for a specific version.
We may want to upgrade from time to time, but we probably don't want the
choice taken out of our hands.

Is there an alternative? Can we put it into a repo? Could I put it on the
nunit.org site as a download?

On Mon, Mar 10, 2014 at 12:59 PM, simoneb notifications@github.com wrote:

Async on .net 4. Not currently, but on the branch of this issue.
On 10 Mar 2014 20:54, "CharliePoole" notifications@github.com wrote:

Remind me what we use NuGet for currently.

On Mon, Mar 10, 2014 at 10:50 AM, simoneb notifications@github.com
wrote:

In any case we're stuck because it's not feasible to get NuGet to work
on
the build server right now due to the old versions on mono installed
both
on Windows and on Linux.

Reply to this email directly or view it on GitHub<
#64 (comment)

.

Reply to this email directly or view it on GitHub<
https://github.com/nunit/nunit-framework/issues/64#issuecomment-37226434>
.

Reply to this email directly or view it on GitHubhttps://github.com//issues/64#issuecomment-37226944
.

@simoneb

This comment has been minimized.

Contributor

simoneb commented Mar 10, 2014

Hi Charlie,

in fact I've already given up with NuGet. I realized that it's basically
impossible to get it to work cross-platform, they probably didn't even
attempt to make it work. Note that on the website they say that it's ok
with mono on Windows, yet you'll find many posts describing hacks and
workarounds.
Because the purpose is just to download a few assemblies for async on .NET
4.0 and because it's just for test projects I currently put the assemblies
in the repo.

On Mon, Mar 10, 2014 at 11:33 PM, CharliePoole notifications@github.comwrote:

I think if we use NuGet, we should be sure to ask for a specific version.
We may want to upgrade from time to time, but we probably don't want the
choice taken out of our hands.

Is there an alternative? Can we put it into a repo? Could I put it on the
nunit.org site as a download?

On Mon, Mar 10, 2014 at 12:59 PM, simoneb notifications@github.com
wrote:

Async on .net 4. Not currently, but on the branch of this issue.
On 10 Mar 2014 20:54, "CharliePoole" notifications@github.com wrote:

Remind me what we use NuGet for currently.

On Mon, Mar 10, 2014 at 10:50 AM, simoneb notifications@github.com
wrote:

In any case we're stuck because it's not feasible to get NuGet to
work
on
the build server right now due to the old versions on mono installed
both
on Windows and on Linux.

Reply to this email directly or view it on GitHub<

#64 (comment)

.

Reply to this email directly or view it on GitHub<
#64 (comment)

.

Reply to this email directly or view it on GitHub<
https://github.com/nunit/nunit-framework/issues/64#issuecomment-37226944>

.

Reply to this email directly or view it on GitHubhttps://github.com//issues/64#issuecomment-37243267
.

@CharliePoole

This comment has been minimized.

Member

CharliePoole commented Mar 10, 2014

That's cool for now.

What if we put any tools we use into the dev repo? It's not used for
anything else and the name fits. We could have a different top-level
directory for each tool and limit exports to the ones we need for each of
our projects.

Charlie

On Mon, Mar 10, 2014 at 4:00 PM, simoneb notifications@github.com wrote:

Hi Charlie,

in fact I've already given up with NuGet. I realized that it's basically
impossible to get it to work cross-platform, they probably didn't even
attempt to make it work. Note that on the website they say that it's ok
with mono on Windows, yet you'll find many posts describing hacks and
workarounds.
Because the purpose is just to download a few assemblies for async on .NET
4.0 and because it's just for test projects I currently put the assemblies
in the repo.

On Mon, Mar 10, 2014 at 11:33 PM, CharliePoole <notifications@github.com

wrote:

I think if we use NuGet, we should be sure to ask for a specific version.
We may want to upgrade from time to time, but we probably don't want the
choice taken out of our hands.

Is there an alternative? Can we put it into a repo? Could I put it on the
nunit.org site as a download?

On Mon, Mar 10, 2014 at 12:59 PM, simoneb notifications@github.com
wrote:

Async on .net 4. Not currently, but on the branch of this issue.
On 10 Mar 2014 20:54, "CharliePoole" notifications@github.com wrote:

Remind me what we use NuGet for currently.

On Mon, Mar 10, 2014 at 10:50 AM, simoneb notifications@github.com
wrote:

In any case we're stuck because it's not feasible to get NuGet to
work
on
the build server right now due to the old versions on mono
installed
both
on Windows and on Linux.

Reply to this email directly or view it on GitHub<

#64 (comment)

.

Reply to this email directly or view it on GitHub<

#64 (comment)

.

Reply to this email directly or view it on GitHub<
#64 (comment)

.

Reply to this email directly or view it on GitHub<
https://github.com/nunit/nunit-framework/issues/64#issuecomment-37243267>
.

Reply to this email directly or view it on GitHubhttps://github.com//issues/64#issuecomment-37245461
.

@simoneb

This comment has been minimized.

Contributor

simoneb commented Mar 10, 2014

Putting this sort of stuff in the dev repo is not ideal because it's files
NuGet downloads when using it from Visual Studio, where it is much easier
to handle them. I would avoid that, I still think we should have the least
possible number of such dependencies and that they should be handled by
some package manager, but NuGet falls short here.

On Tue, Mar 11, 2014 at 12:03 AM, CharliePoole notifications@github.comwrote:

That's cool for now.

What if we put any tools we use into the dev repo? It's not used for
anything else and the name fits. We could have a different top-level
directory for each tool and limit exports to the ones we need for each of
our projects.

Charlie

On Mon, Mar 10, 2014 at 4:00 PM, simoneb notifications@github.com wrote:

Hi Charlie,

in fact I've already given up with NuGet. I realized that it's basically
impossible to get it to work cross-platform, they probably didn't even
attempt to make it work. Note that on the website they say that it's ok
with mono on Windows, yet you'll find many posts describing hacks and
workarounds.
Because the purpose is just to download a few assemblies for async on
.NET
4.0 and because it's just for test projects I currently put the
assemblies
in the repo.

On Mon, Mar 10, 2014 at 11:33 PM, CharliePoole <notifications@github.com

wrote:

I think if we use NuGet, we should be sure to ask for a specific
version.
We may want to upgrade from time to time, but we probably don't want
the
choice taken out of our hands.

Is there an alternative? Can we put it into a repo? Could I put it on
the
nunit.org site as a download?

On Mon, Mar 10, 2014 at 12:59 PM, simoneb notifications@github.com
wrote:

Async on .net 4. Not currently, but on the branch of this issue.
On 10 Mar 2014 20:54, "CharliePoole" notifications@github.com
wrote:

Remind me what we use NuGet for currently.

On Mon, Mar 10, 2014 at 10:50 AM, simoneb <
notifications@github.com>
wrote:

In any case we're stuck because it's not feasible to get NuGet to
work
on
the build server right now due to the old versions on mono
installed
both
on Windows and on Linux.

Reply to this email directly or view it on GitHub<

#64 (comment)

.

Reply to this email directly or view it on GitHub<

#64 (comment)

.

Reply to this email directly or view it on GitHub<

#64 (comment)

.

Reply to this email directly or view it on GitHub<
#64 (comment)

.

Reply to this email directly or view it on GitHub<
https://github.com/nunit/nunit-framework/issues/64#issuecomment-37245461>

.

Reply to this email directly or view it on GitHubhttps://github.com//issues/64#issuecomment-37245684
.

simoneb added a commit that referenced this issue Mar 12, 2014

simoneb added a commit that referenced this issue Mar 12, 2014

CharliePoole added a commit that referenced this issue Mar 12, 2014

Merge pull request #117 from nunit/issue-64
Checking AsyncStateMachineAttribute by name only. #64

I see no reason not to just merge this, since it should work just the same as the original code.

@simoneb simoneb closed this Mar 14, 2014

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