return original value when no cast

[shmax]

Fix for #533

This change aligns TypeConstraint::toString with the other casting methods in that if it can't figure out a cast to do, then it simply returns the original value.

[shmax]

@erayd @LordWeedlle

[LordWeedlle]

Damn, didn't noticed only toString had the problem ^^
Seems working, nice job =)

[shmax]

This is the only new code in this file. Everything else is just redoing the numbers (why do we have numbers in the comments, again?)

[LordWeedlle]

I presume numbers are there for debug purpose, so when phpunit tells you testXXX with dataset #23 failed, you don't have to count arrays to find the one failing XD

[erayd]

Yep, that's what they are for.

[shmax]

But that's awful, isn't it? Shouldn't PHPUnit be giving us line numbers? I just tried it locally, and it does report line numbers, but they're wrong. Is this really considered idiomatic test code, or are we just cargo culting?

[erayd]

PHPUnit can't give us useful line numbers, because it doesn't know where the test is defined.

We can set names for the datasets and PHPUnit can report those, but we don't do that currently - there was a reason, but I don't recall what it was (backwards compatibility with something perhaps?), hence the numbers. They serve their purpose, but are annoying to maintain.

If you want to try naming things and see if that's an option now, feel free :-).

[shmax]

Right, but you said it's not echoing the comment. If it's not echoing the comment, then there's nothing for you to eyeball. If it is echoing the comment, and you're only seeing that comment, then it seems like a line number could be derived, somehow.

[erayd]

I think you are misunderstanding how this works.

PHPUnit reports the array index of the test, but doesn't say anything about what that test is, or which line number it's defined on. In order to locate where the failing test is defined, one must manually search through the source file and find the test with the correct array index.

The comments are manually-added references to the array index of each test. That way, instead of having to manually count through the entire array definition in order to locate which test PHPUnit just complained about, you can simply look for the comment with the correct number.

Ideally, we'd use associative indexes with a useful description in them, which would render the comments redundant - I wanted to do this at one point, but there was a technical reason why it wasn't possible at that time - I do not recall why, but it may have been a compatibility conflict. If you would like to look into this again, you are welcome to do so - it may well be that whatever the issue was, has been resolved since I last looked into it for this project.

[shmax]

Oh, is that what's going on? Egad, it's even worse than I thought. Well, shouldn't be too tough to fix. Here's one idea:

protected function _addTest($config) {
  $config[] = debug_backtrace()[0]['line'];
  return $config;
}

Use it like this:

$tests[] = $this->_addTest(array(
            '{"properties":{"propertyOne":{"type":["number", "string"]}}}',
            '{"propertyOne":42}',
            'integer', 'integer', 42, true
        ));

Then you modify your batch test method to expect the line number:

public function testCoerceCases($schema, $data, $startType, $endType, $endValue, $valid, $lineNumber, $extraFlags = 0, $assoc = false)
    {

From there I suppose you could just write some kind of helper assert method that appends the line number.

[erayd]

Oh, is that what's going on? Egad, it's even worse than I thought.

Yep, it's complete shit, and I'm responsible for introducing it. Sorry ;-).

Your idea is a good one; I hadn't considered using the backtrace. I think we should look at providing that info as part of an assoc index first (rather than using a special function), because that would be IMO a better place for that information if it's now usable, and will result in PHPUnit printing it alongside the error. If we still can't set an assoc index though, then we'll need to modify the batch tests as you mention above.

[shmax]

Sure, either idea sounds like an improvement. 👍

[erayd]

LGTM 👍

[LordWeedlle]

yup, seems good to me too, gj!

[LordWeedlle]

I didn't took time to thank you for your rapidity and your work @shmax @erayd so I do it now =)
Thank you !
It's always a pleasure to use a repository where people are reactive !

[shmax]

thank you for your rapidity and your work @shmax @erayd

Pleasure to be of service. 😄