QualiSystems / FluentTc

:ocean: :two_men_holding_hands: :office: Integrate with TeamCity fluently
https://www.nuget.org/packages/fluenttc
Apache License 2.0
44 stars 36 forks source link

Property type builders, tests #57

Closed sokolovsv90 closed 8 years ago

sokolovsv90 commented 8 years ago

As discussed in pull request #48 All tests passed

borismod commented 8 years ago

@sokolovsv90 are you sure the build passes?

sokolovsv90 commented 8 years ago

@borismod why do you ask? It does and all the tests pass. I almost feel like I'm missing something

borismod commented 8 years ago

For some reason there are $ instead of @

if (!validationMap.ContainsKey(validation))
 +                Assert.Inconclusive($"Wrong {nameof(validation)} argument value passed");
 +            validationMap[validation]();

http://teamcity.codebetter.com/viewLog.html?buildId=215162&tab=buildResultsDiv&buildTypeId=FluentTc_FluentTcDevelop

image

sokolovsv90 commented 8 years ago

Oh... theese are special characters for string interpolation introduced in c# 6.0. I certainly can fix that, which version of msbuild do You use?

borismod commented 8 years ago

Microsoft Visual Studio 2013

sokolovsv90 commented 8 years ago

Should work now

borismod commented 8 years ago

@sokolovsv90 it get's better:

image

sokolovsv90 commented 8 years ago

bah, forgot that this was also a feature of c#6 :)

sokolovsv90 commented 8 years ago

Should work now v.2

borismod commented 8 years ago

@sokolovsv90 this is a great contribution and this is exactly the way like I would like to see it. just a few minor comments:

  1. Replace Append with Append format
  2. Use const instead of inline strings
  3. Replace Verify.That with FluentAssertions
  4. Make sure all new code is covered by unit tests. For example, the following methods are not: BuildParameterTypeBuilder.AsCheckbox and BuildParameterTypeBuilder .AsText
  5. Add at least one Acceptance Test for each type: Select, CheckBox, Password, Normal. The below edge case is not unit tested: image

I am sorry for being so "hard" and I know you did a great job. I'm just trying to keep high standards in this project. I hope you'll understand

borismod commented 8 years ago

@sokolovsv90 I assume that I was too hard to contributors. I really appreciate your contribution and I will accept your pull request as is. Can you just resolve the conflicts?

sokolovsv90 commented 8 years ago

I understand your concerns and totally agree with you in terms of keeping high standards, but I simply haven't noticed how my own stuff got piled up and owerwhelmed me a bit. I'm truly sorry that I couldn't meet all the requirements (and I wish I had time to), but I'm glad you accepted that PR as is, thanks!

borismod commented 8 years ago

@sokolovsv90 FluentTc was published to nuget with your contribution, so you're welcome to install its latest package. I would like to thank you for your contribution and will be glad to see you contributing in the future. I'll try to be nicer :smile:

borismod commented 8 years ago

@sokolovsv90 what's your full name, so I could give you a credit on contributors list?