Closed flohw closed 1 year ago
hello @flohw
indeed our tests are missing a case where the value is an int, see https://github.com/zenstruck/browser/blob/1.x/tests/JsonTest.php#L68
You mean Assert::that($this->search($selector))->notEmpty()
, right?
because if you do Assert::that($selector)->isNotEmpty()
it will just test the string $selector
(in your case total
).
I don't remember why we used length()
here. I remember that we couldn't return true
here on null
values, but it is by design in JMESPath spec :disappointed:
Then I'd say the solution would be to use isNotNull()
because we want empty string values to return true
here.
We should also change assertMissing()
as well
PRs are welcome, of course, thanks :smile:
Sorry, I only have isNotEmpty()
with my auto completion on Assert::that
zenstruck/browser 1.2.0
zenstruck/assert 1.2.0
My changes in Zenstruck\Browser\Json
: (original code commented)
/**
* @param string $selector JMESPath selector
*/
public function assertHas(string $selector): self
{
Assert::that($selector)->isNotEmpty();
// Assert::try(
// fn() => $this->search("length({$selector})"),
// 'Element with selector "{selector}" not found.',
// ['selector' => $selector]
// );
return $this;
}
I will try, I am fighting with uninitialized entities properties, not sure if it's due to doctrine or foundry for now and don't understand why...
Assert::that($selector)->isNotEmpty();
this code won't work, you're testing the string $selector
. $json->assertHas('not existing selector')
will not fail since the string is not empty
BTW, zenstruck/assert 1.2.0 is the last version, isNotNull()
exists there: https://github.com/zenstruck/assert/blob/v1.2.0/src/Assert/Expectation.php#L83
sorry, I don't understand your Foundry problem. both versions seems similar to me
Ho I missed the $this->search
call...
So both works: isNotEmpty
and isNotNull
and I do agree that isNotNull
should be used.
I will provide a PR. How would be assertMissing
be changed?
No problem for the other issue, it looks the same to me too but don't behave the same. ^^ Thanks.
Found with isNull
.
I added tests on these empty-ish values:
yield ['{"foo": 1}', 'foo'];
yield ['{"foo": ""}', 'foo'];
yield ['{"foo": 0}', 'foo'];
yield ['{"foo": null}', 'foo'];
yield ['{"foo": false}', 'foo'];
The only remaining problem is for {"foo": null}
as null could be return because of search
or because search
didn't found the selector... Any idea how to resolve this edge case?
The only remaining problem is for {"foo": null} as null could be return because of search or because search didn't found the selector... Any idea how to resolve this edge case?
I think for these assertions we have to assert it has the a value (using Json::assertHas()
) before checking if it empty/null/not-null/not-empty.
yield ['{"foo": 1}', 'foo'];
~This isn't an empty-ish value.~
Edit: I think I see what you're meaning and understand the conundrum.
assertHas
has the same issue as the value is null
it's considered missing.
And assertMissing
don't find the value as it's null
and the exception is not thrown.
Yes I added this test because of my initial problem : length
was not working on int.
I doubt we will be able to distinguish "null value" and "node not present" with JMESPath
see this comment: I kinda agree when he/she says:
the specification is broken by design
So I remove the test and provide the PR. :slightly_smiling_face:
Yeah... I'm not seeing a great solution here. @nikophil is right, it isn't part of the spec but lots of discussion around this.
It is possible using the keys
and contains
function but not sure we can do this in a generic way (by just accepting a foo.bar.baz
$selector
). Maybe I'm wrong?
https://stackoverflow.com/a/51519959 is what I'm referring to.
I think this needs some digging...
@flohw are you willing do to that in your PR? if no, no problem, I'll merge your PR and create another issue
I tried to do some things base on some explode
which didn't work and produce an ugly code. ^^
I am not sure how it could be done differently or how to combine the different Assert possibilities.
And my other bug which may require me to update all my tests is painful.
Leave my pr open, I'll try to search a bit more on monday. If you find a solution during the weekend you can close my pr and create your own or provide the diff and I will be able to apply in my branch. I am already happy to be able to help a bit here anyway. :slightly_smiling_face:
Thank you.
I don't see other way around than using explode
.
Or maybe a regex will do the job, not sure it's less ugly :upside_down_face: https://regex101.com/r/DOqsmk/1 https://3v4l.org/Yeg0m
I'm also wondering what would be the syntax if the node is a root node. ie: {"total": null}
explode
will be fine and easier to read. :sweat_smile:
I saw @
somewhere but didn't tried and not sure how to use it. Maybe contains(keys(@), 'total')
:thinking:
yes it works!
you got a playground for jmespath here: https://jmespath.org/
@kbond do you see some edge cases? I don't know if we can pass to assertHas()
and assertMissing()
another kind of selector than simple ones like that foo[0].bar.baz
That's what I've been thinking too - should we only support .
/[x]
notation here?
If so, we should have tests for:
foo
foo.bar
foo.bar.baz
[0]
foo[0].bar
foo[0][1]
I soooo want this: https://github.com/symfony/symfony/issues/48067
Hello there, With one day late, I am working a bit on this.
I managed to make it work for all of your cases @kbond except for [0]
and foo[0][1]
because keys
function expect to have an object and in both cases it's an array.
The test cases are yield ['[null]', '[0]'];
and yield ['{"foo": [[1, null]]}', 'foo[0][1]'];
. All the added tests are on a null
value.
The method look like this:
public function assertMissing(string $selector): self
{
$prefix = 1 === preg_match('#^\[#', $selector) ? '' : '@.';
$exploded = explode('.', $prefix.$selector);
$key = array_pop($exploded);
$keySelector = implode('.', $exploded);
Assert::that($this->search("contains(keys($keySelector), '$key')"))
->equals(false, 'Element with selector "{selector}" exists but it should not.', ['selector' => $selector]);
return $this;
}
I am not sure about adding the @.
but this is the only way I found to prevent multiple ifs statements.
Also the $prefix
which is empty or '@.'
is required because selector like this one: yield ['[{"foo":"bar"}]', '[0].foo'];
doesn't work with @.[0]
nor @[0]
but does with [0]
.
Only two edge cases remain: [0]
and foo[1][0]
which may not be logical in this context or at least have workarounds:
->assertThat('length(@)', 0)
and ->assertThat('length(foo[0][1])', 0)
->assertThat("!contains([0], 'needle')")
and ->assertThat("!contains(foo[0][1], 'needle')")
.Given the names has
and missing
maybe it's ok as for arrays we usually see contains
which will require a second argument on both of the existing methods doesn't provide: the needle.
What do you think? Any advice about the last bit ([0]
and foo[0][1]
)?
Note: The method assertHas
with exactly the same code (except that equals
tests on true
) works as well as this one.
Hi @flohw
would you push the code on your PR please? we could explicitly see the failing cases
thanks!
Hi, I just pushed the changes.
Hi,
I just updated the library in my project to latest version (1.2) and replaced some places to use
KernelBrowser::browser()->json()->assertHas('selector')
. But it seems that it doesn't work as expected.I'm testing a json api endpoint which returns this json:
Here is my test which fails:
Here is my test which succeeds:
I tried to replace the
Assert::try
inJson::assertHas
byAssert::that($selector)->isNotEmpty()
and it works. I think the problem comes fromlength
used which in this case returns something empty as1
is not countable... I don't know if my solution is the best to use. If so, let me know I can provide the PR. :-)Thank you