Added Tests for Setting Element#27807
Conversation
|
|
|
Hi @Jlalond thanks a lot for your first contribution! Could you please update to match |
| public void AssureDefaultValueIsNull() | ||
| { | ||
| SettingElement Element = new SettingElement(); | ||
| Assert.Equal(Element.Value.CurrentConfiguration, null); |
There was a problem hiding this comment.
In Assert.XXX overloads like Assert.Equal, the first parameter is the "expected" and the second parameter is the "actual". So these parameters should be reversed, in order to get accurate error messages if the assert fails.
| [Fact] | ||
| public void TestForInequality() | ||
| { | ||
| SettingElement ElementOne = new SettingElement("NotEqualOne", SettingsSerializeAs.String); |
There was a problem hiding this comment.
Nit, per the coding guidelines, we generally use "var" iff the right side is a constructor of the same type. Basically if you are assigning "new Foo(..)" it's almost always into a variable of type "Foo" so it's OK to use "var" and we generally do that. So these SettingElement can be var.
|
|
||
| public class SettingElementTests | ||
| { | ||
|
|
There was a problem hiding this comment.
Nit extra line here. Also, we put using statements outside of namespace.
| @@ -0,0 +1,69 @@ | |||
| namespace System.Configuration | |||
There was a problem hiding this comment.
We put tests in a namespace suffixed ".Tests"
| } | ||
|
|
||
| [Fact] | ||
| public void AssureDefaultNameIsEmptyString() |
There was a problem hiding this comment.
Nit, I think "Assure" should probably be "Ensure". Or, many people would just miss off the verb at the front.
| Value = new SettingValueElement | ||
| { | ||
| ValueXml = new ConfigXmlDocument | ||
| { |
There was a problem hiding this comment.
If you're not passing parameters I would not use an initializer (curlies) but instead regular parentheses on the same line.
There was a problem hiding this comment.
Hey Dan, this is the only one I was confused about what you mean/how to do this without the initlializer
There was a problem hiding this comment.
I meant you can just do new ConfigXmlDocument() since you aren't setting parameters you don't need the initializer list ({.... })
It's not a big deal though - we can merge like this.
|
Only a few comments - looks good otherwise! When you've had a chance to fix, someone will sign off and merge for you. |
You are probably right. We generally do not throw NRE intentionally. However this code is old, and it is probably best not to change its behavior now. |
| { | ||
| var Element = new SettingElement(); | ||
| Assert.Throws<NullReferenceException>(() => Element.GetHashCode()); | ||
| //likely a bug |
There was a problem hiding this comment.
I'm fine with you fixing this, and would actually prefer it if you would (assuming you're ok with doing so).
There was a problem hiding this comment.
I'm up for it, I figured I'd start with Tests to get used to code style and get some feedback on the cleanliness of my code (point of work it seems)
Fixing bugs sounds fun though
danmoseley
left a comment
danmoseley
left a comment
There was a problem hiding this comment.
Thanks! If you'd like to fix the bug, please open an issue and go ahead and offer a PR for it.
* Added Tests for Setting Element * Cleaning as per the corefx standards * including .net license Commit migrated from dotnet/corefx@7e64364
5 participants
For #15554
I did a small test class here, to get feed back on code style and how the corefx repo does it's unit tests.
While it's my first PR I might as well say I'm pretty new to the industry, and I'm here to contribute to the project in order to learn and grow as a developer. So feedback is appreciated!
Also, GetHashValue for a SettingElement throws a NullReferenceException, and I think that might be a bug.