Skip to content
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

HaveElementWithValue? #39

Open
weitzhandler opened this issue Jul 27, 2020 · 18 comments
Open

HaveElementWithValue? #39

weitzhandler opened this issue Jul 27, 2020 · 18 comments

Comments

@weitzhandler
Copy link

Hi,

I wish there was a way to assert a JObject contains an element with a value as specified.

// arrange
...
var expectedTokenKey = "token";
var expectedTokenValue = "1234";

// act
JObject json = await ...GetJsonAsync();

// assert
json.Should().HaveElementWithValue(expectedTokenKey, expectedTokenValue);

Is there another better way for that I'm unaware of?

@dennisdoomen
Copy link
Member

No, but we do accept contributions ;-)

weitzhandler added a commit to weitzhandler/fluentassertions.json that referenced this issue Jul 28, 2020
weitzhandler added a commit to weitzhandler/fluentassertions.json that referenced this issue Jul 28, 2020
weitzhandler added a commit to weitzhandler/fluentassertions.json that referenced this issue Jul 28, 2020
@jnyrup
Copy link
Member

jnyrup commented Jul 28, 2020

In the FluentAssertions core library I can only think of
GenericDictionaryAssertions.Contain(TKey key, TValue value) that does a similar combined assertion.

dictionary.Should().Contain(1, "One");

which could also be written as

dictionary.Should().ContainKey(1)
    .WhichValue.Should().Be("One");

In FluentAssertions.Json the proposed HaveElementAndValue(string expectedElement, string expectedValue)

var subject = JToken.Parse("{ 'id': 42 }");

subject.Should().HaveElementAndValue("id", "42");

can also be written as either

var subject = JToken.Parse("{ 'id': 42 }");

subject.Should().HaveElement("id")
    .Which.Should().HaveValue("42");

or

var subject = JToken.Parse("{ 'id': 42 }");

subject.Should().HaveElement("id")
    .Which.Value<string>().Should().Be("42");

Using the existing Which pattern has two benefits:

  • has no implicit assumptions that the Value of the id element is convertible to string
  • the same code pattern can be used for other expectations than string.
var subject = JToken.Parse("{ 'id': [42] }");

subject.Should().HaveElement("id")
    .Which.Values<int>().Should().ContainSingle()
        .Which.Should().Be(42);

weitzhandler added a commit to weitzhandler/fluentassertions.json that referenced this issue Jul 28, 2020
@weitzhandler
Copy link
Author

Not only do I like it, but I wasn't even aware of the Which operator, which I was so anxiously looking for 😳

Thank you 💐

@weitzhandler
Copy link
Author

Related: fluentassertions/fluentassertions#1355.

@dennisdoomen
Copy link
Member

But does that mean we still see value in #39?

@jnyrup
Copy link
Member

jnyrup commented Jul 29, 2020

I haven't really used FA.Json that much, but I'm not seeing cases for HaveElementAndValue that aren't already covered by the existing API.

@weitzhandler
Copy link
Author

The use case I encountered is just like you described with the dictionary value.
I get json from HTTP API server. I want to verify the json contains a specified key and value.

@dennisdoomen
Copy link
Member

But would subject.Should().HaveElement("id").Which.Value<string>().Should().Be("42"); work for you?

@weitzhandler
Copy link
Author

It actually would. A syntax sugar would be nice tho. Like in dictionary, as Jonas mentioned.

@jnyrup
Copy link
Member

jnyrup commented Jul 29, 2020

I haven't dug into the history of Contain(TKey, TValue), so I'm possibly missing some historical context, but I'm not that big fan of it, as it:

  • tries to assert multiple things at once, and
  • can be accomplished using the WhichValue

To be clear, the existence of Contain(TKey, TValue) (in my opinion) is not the right argument to use to justify the addition of HaveElementAndValue.

@dennisdoomen
Copy link
Member

But I don't mind it either. Having a useful shortcut is fine by me.

@weitzhandler
Copy link
Author

But I don't mind it either. Having a useful shortcut is fine by me.

Then we're left with the naming to be decided. Let's continue this conversation in #40.

@dennisdoomen
Copy link
Member

The naming is fine by me.

@jnyrup
Copy link
Member

jnyrup commented Jul 30, 2020

I'm a bit confused whether we're agreeing on HaveElementWithValue or HaveElementAndValue?

@dennisdoomen
Copy link
Member

I think there's two discussions:

  1. Whether or not to add this API at all.
  2. What the name should be.

I know that we can accomplish the same with a combination of HaveElement and Which, but I don't mind adding this new member. I would prefer to name it HaveElementWithValue.

@jnyrup
Copy link
Member

jnyrup commented Jul 30, 2020

After a nights sleep, I'm fine adding it.
I agree that HaveElementWithValue is preferable to HaveElementAndValue.

Regarding the method signature, I'm looking at https://en.wikipedia.org/wiki/JSON#Data_types_and_syntax to get a picture of possible scenarios.

  • Should it be generic HaveElementWithValue<T>(string expectedElement, T expectedValue)?
    • That won't work for Array type, as those should be extracted using Values<T> instead of Value<T>
  • Should we have bool, double, string overloads
  • Should it be able to handle Object

@dennisdoomen
Copy link
Member

Json is text only, so why don't we stick to a string and a Jtoken overload?

@weitzhandler
Copy link
Author

  • Should it be generic HaveElementWithValue<T>(string expectedElement, T expectedValue)?

I like this.

Json is text only, so why don't we stick to a string and a Jtoken overload?

Eventho JSON is text only, 5 isn't recognized as "5". Similarly null != "null", in the JSON.

weitzhandler added a commit to weitzhandler/fluentassertions.json that referenced this issue Aug 3, 2020
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