-
Notifications
You must be signed in to change notification settings - Fork 34
Implementing GetProject() with lambda calls #107
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
base: master
Are you sure you want to change the base?
Conversation
borismod
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WiseLordship thanks a lot for your contribution. I'll appreciate if you could take a loot on my questions.
| A.CallTo(() => teamCityCaller.Get<Project>(expectedCall)).MustHaveHappened(); | ||
| } | ||
|
|
||
| private static TeamCityCaller CreateTeamCityCaller() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not reusing CreateTeamCityCaller from AcceptanceTests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started with that and realized I was not mocking it the right way. So moved that out and new tests should be following the mocking correctly.
FluentTc/Engine/ProjectsRetriever.cs
Outdated
| } | ||
|
|
||
| public Project GetProject(string projectId) | ||
| public Project GetProject(Action<IBuildProjectHavingBuilder> having = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why having is nullable? What is the expected result when we call GetProject() ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I was not fully looking at it correctly. I added the logic to make sure we are only returning one and made it similar to GetAgent().
| [TestCase("FluentTC")] | ||
| [TestCase("Test 1")] | ||
| [TestCase("Test%20C")] | ||
| public void GetProject_ByName(string projectName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this test covers URL encoding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually just testing random characters and thought that would be the most common that someone would enter. Currently, not handling URL encoding.
| namespace FluentTc.Tests | ||
| { | ||
| [TestFixture] | ||
| public class ProjectRetrievalTests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are acceptance tests and they should reside inside AcceptanceTests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will move these to AcceptanceTests and will make better overall tests for the method. I started here when I was trying t make more robust tests, and did not move these out.
|
@WiseLordship what is the status of this PR? |
Issue details
Implementation for getting projects based on lambda expressions.
Relates to issue
#102
Checklist
Example of using new/modified functionality