Search/Users Api

[hahmed]

#276

• Refactor to use BaseSearchRequest (Base class for searching GitHub #301)
• Fix those failing tests

[hahmed]

@shiftkey ready for review 😄

[shiftkey]

These tests are failing because the .GetAll<Repository> should be .GetAll<User>

[hahmed]

My bad - that's been fixed now.

[shiftkey]

@hahmed one of the things I'd like to challenge you on is how those unit tests actually validate the parameters you set in the query.

So we have a test like this:

[Fact]
public void TestingTheInQualifier()
{
    var connection = Substitute.For<IApiConnection>();
    var client = new SearchClient(connection);
    //get users where the fullname contains 'github'
    var request = new SearchUsersRequest("github");
    request.In = new[] { UserInQualifier.Fullname };
    client.SearchUsers(request);
    connection.Received().GetAll<User>(Arg.Is<Uri>(u => u.ToString() == "search/users"), Arg.Any<Dictionary<string, string>>());
}

We know that the q value on the querystring parameter should contain something, so why not check it within the test, like this?

connection.Received().GetAll<User>(
    Arg.Is<Uri>(u => u.ToString() == "search/users"),
    Arg.Is<Dictionary<string, string>>(d => d["q"] == "???"));

cc @alfhenrik as this is relevant to #290

[hnrkndrssn]

👍

[hahmed]

When i get a moment tomorrow i will look at this. I agree the qualifiers should be tested a bit better.

[hahmed]

We know that the q value on the querystring parameter should contain something, so why not check it within the test, like this?

@shiftkey I agree - I will implement this as per your suggestion.

[shiftkey]

This should be "something"

[hahmed]

@shiftkey thanks, nearly ready for review now, just updated all tests. I am rebasing now too.

[hahmed]

@shiftkey ready for review.

[shiftkey]

A 💄 change - bring this back up to the previous line and let the : breathe with some whitespace

[shiftkey]

There's a few failing tests here because the enum types, such as Language and AccountType, that don't specify the parameter values necessary to map them to the lower-case values...

[hahmed]

@shiftkey thank's will fix them, to be honest I cannot see any failing tests (I mentioned this last time), everything just keeps on passing for some reason, I need to look into why I cannot see any failing tests, something must be broken with my scripts (I never made any updates to that at all).

[shiftkey]

@hahmed if you're running .\build you might have hit this issue #353

Alternatively, you can run .\script\cibuild.ps1 which is the same script the build server runs (takes a bit longer)

[hahmed]

@shiftkey now that I have merged into master, I can run .\build,cmd and I can see 11 failing tests - oh my 😄

[hahmed]

@shiftkey that's all done now. All tests are passing, anything else I missed, please let me know.

[shiftkey]

Thanks!

[hahmed]

Lol... amazing!