Skip to content

Change Error Message for Assert.Equals #1954

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
rprouse opened this issue Jan 4, 2017 · 12 comments · Fixed by #1957
Closed

Change Error Message for Assert.Equals #1954

rprouse opened this issue Jan 4, 2017 · 12 comments · Fixed by #1957

Comments

@rprouse
Copy link
Member

rprouse commented Jan 4, 2017

Based on the number of votes on this StackOverflow question, http://stackoverflow.com/questions/11584429/nunits-assert-equals-throws-exception-assert-equals-should-not-be-used-for-ass, many users find the message in our Assert.Equals override to be confusing. It currently says "Assert.Equals should not be used for Assertions", we should change it to be "Assert.Equals should not be used for Assertions, use Assert.AreEqual(...) instead".

@JustinRChou
Copy link
Contributor

Should the summary and ReferenceEquals also be updated?

@CharliePoole
Copy link
Member

@JustinRChou That makes sense
@rprouse Since we are unable to use EditorBrowsable in this context, I suggest that the intellisense should start with something like "DO NOT USE"!

@rprouse rprouse reopened this Jan 4, 2017
@CharliePoole
Copy link
Member

Whoops! 😄

@JustinRChou
Copy link
Contributor

Also looks like following classes needs updating too.

Assume
CollectionAssert
DirectoryAssert
FileAssert
StringAssert
Warn

And subsequent test cases.

@CharliePoole
Copy link
Member

@rprouse Can of worms? 😄 I think most of this can wait till after the release though.

@JustinRChou
Copy link
Contributor

When is the next release?
And I have updated as such before I start applying it to the rest.

        /// <summary>
        /// DO NOT USE! Use Assert.AreEqual(...) instead.
        /// The Equals method throws an InvalidOperationException. This is done 
        /// to make sure there is no mistake by calling this function.
        /// </summary>
        /// <param name="a"></param>
        /// <param name="b"></param>
        [EditorBrowsable(EditorBrowsableState.Never)]
        public static new bool Equals(object a, object b)
        {
            throw new InvalidOperationException("Assert.Equals should not be used for Assertions, use Assert.AreEqual(...) instead.");
        }

And should the EditorBrowsable attribute be applied to ReferenceEquals? I am not too familiar on how the attribute works.

@CharliePoole
Copy link
Member

@JustinRChou In a few days.
I like what you have done as a comment. I would usually drop the param elements if not using them. Are you planning to do a PR?

After much experimentation, I believe we have found that EditorBrowsable is no help at all for these messages. Feel free to experiment further. 😄

@JustinRChou
Copy link
Contributor

Sure.

Might not have to update the test cases since those use StartsWith.

JustinRChou added a commit to JustinRChou/nunit that referenced this issue Jan 5, 2017
Update summary and exception of overriden Equals and ReferenceEquals
with "DO NOT USE" and suggestions of the correct methods to use.
@JustinRChou
Copy link
Contributor

@nunit/core-team
Created the PR.
And am I doing something wrong with my fork? I seem to have way more merge commits showing up.
Do I need to use the merge with the squash commit option?

@CharliePoole
Copy link
Member

I commented on the PR. I'm a little confused about what you did. You only created the PR an hour ago, how did it end up having so many things merged into it? Did you create your branch from an updated master branch?

@JustinRChou
Copy link
Contributor

That was what I am trying to figure out.

Before that, I was updating my fork with the new updates and I created another branch from that for the PR.

So I am guessing that I am doing something incredibly wrong in my fork.

JustinRChou added a commit to JustinRChou/nunit that referenced this issue Jan 5, 2017
Update summary and exception of overriden Equals and ReferenceEquals
with "DO NOT USE" and suggestions of the correct methods to use.
@JustinRChou
Copy link
Contributor

Rebased my fork and recreated the PR.

JustinRChou added a commit to JustinRChou/nunit that referenced this issue Jan 6, 2017
Removed incorrect suggestions for ReferenceEquals.

Changed summary of ReferenceEquals to match the style of Equals.

Added . to exception messages.
rprouse added a commit that referenced this issue Jan 9, 2017
JustinRChou added a commit to JustinRChou/nunit that referenced this issue Jan 9, 2017
Update summary and exception of overriden Equals and ReferenceEquals
with "DO NOT USE" and suggestions of the correct methods to use.
JustinRChou added a commit to JustinRChou/nunit that referenced this issue Jan 9, 2017
Removed incorrect suggestions for ReferenceEquals.

Changed summary of ReferenceEquals to match the style of Equals.

Added . to exception messages.
@rprouse rprouse added this to the 3.6 milestone Jan 9, 2017
JustinRChou added a commit to JustinRChou/nunit that referenced this issue Jan 9, 2017
* Added tests for zero-case tests

* Implemented fix to allow zero-case tests to pass

* Simplified parameterize condition to any test builder

* Correct tests to no longer expect non-runnable when there is no data provided

* Use TestParametersDictionary if present rather than TestParameters

* NUnitLite taking responsibility for parsing test params

* Made ExactCount standalone and added Items no-op

* Added Exactly syntax tests

* nunit#1954 Assert Equals Error Message

Update summary and exception of overriden Equals and ReferenceEquals
with "DO NOT USE" and suggestions of the correct methods to use.

* Add AttributeUsage to two attributes

* nunit#1954 Update for Review

Removed incorrect suggestions for ReferenceEquals.

Changed summary of ReferenceEquals to match the style of Equals.

Added . to exception messages.

* Fixed typos in comments incl XML docs

* Fixed typos in private methods and test names

* Fixed typo in exception message

* Additional tests for pr 1942

* Fix test errors

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

Successfully merging a pull request may close this issue.

3 participants