Skip to content

TestFixtureTests.CapturesArgumentsForConstructorWithMultipleArgsSupplied assumes order of custom attributes #2132

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

Closed
oznetmaster opened this issue Apr 24, 2017 · 5 comments

Comments

@oznetmaster
Copy link
Contributor

This test assumes that the order of the Tests in the TestFixture is defined.

It is not.

The order is based on the order of the Type.GetCustomAttributes return value, which is documented to be random.

Therefore, no test should be written that assumes anythings based on the order of the return value from Type.GetCustomAttributes.

This test fails on some platforms.

The lexical ordering of elements in a file is absolutely not guaranteed to be persisted in anyway in the resulting CIL assemblies nor to be respected in the results returned from Reflection. This ordering is not even guaranteed to be the same over repeated calls within the same app domain!

Note that MS have broken this ordering in other parts of reflection in the past (Noting as they did it that this actually caused some issues for some of their code), thus even if it happens to work at the moment there is nothing stopping this breaking in future or on different platforms.
@CharliePoole
Copy link
Member

Of course, you are correct. This is well known to the nunit devs and is even documented for users. However, this was done by a contributor about a month ago. It's up to us to catch such things, which may be a bit esoteric to those who make contributions. I encourage everyone to review code for contributors!!!

@CharliePoole
Copy link
Member

@Suremaker Would you care to put this right? There should be two tests and one of them should have one set of args while the other has the second set. Order is indeterminate, as @oznetmaster points out, even between different versions of Microsoft runtimes and may be all the more so when using non-MS runtimes.

Suremaker added a commit to Suremaker/nunit that referenced this issue Apr 25, 2017
…leArgsSupplied assumes order of custom attributes
@Suremaker
Copy link
Contributor

Hello, I have changed the test to assert captured arguments in with ignoring the order. Please let me know if further changes are needed.

oznetmaster added a commit that referenced this issue Apr 25, 2017
#2132 TestFixtureTests.CapturesArgumentsForConstructorWithMultipleArgsSupplied assumes order of custom attributes
@CharliePoole
Copy link
Member

Fixed by PR #2138

@CharliePoole
Copy link
Member

Thanks for fixiing this @Suremaker

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

No branches or pull requests

3 participants