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

Throw exception when @testWith annotation contains invalid datasets #1968

Conversation

acoulton
Copy link
Contributor

The existing code was silently ignoring invalid JSON if it had
found at least one valid dataset. It was using this behaviour
to detect the end of the @testwith annotation (see for eg the
testTestWithSimpleTextAfter test, which fails if a simple
throw on invalid JSON is used).

This fix treats a line as being part of the @testwith if it begins
with a [ character - since all datasets are required to be JSON
arrays this should be reliable.

It can then reliably throw if invalid JSON is found for any of the
datasets.

@giorgiosironi
Copy link
Contributor

What about including json_last_error_msg() in the exception message in the PHP versions where this function is available (checking with function_exists)?

@acoulton acoulton force-pushed the bug/report-testwith-parse-errors branch 4 times, most recently from ab8b0ea to 728bc71 Compare March 21, 2016 14:22
The existing code was silently ignoring invalid JSON if it had
found at least one valid dataset. It was using this behaviour
to detect the end of the @testwith annotation (see for eg the
testTestWithSimpleTextAfter test, which fails if a simple
throw on invalid JSON is used).

This fix treats a line as being part of the @testwith if it begins
with a `[` character - since all datasets are required to be JSON
arrays this should be reliable.

It can then reliably throw if invalid JSON is found for any of the
datasets.
@acoulton acoulton force-pushed the bug/report-testwith-parse-errors branch from 728bc71 to 9d861b3 Compare March 21, 2016 14:58
@acoulton
Copy link
Contributor Author

@giorgiosironi thanks for the suggestion, that's a good idea - I've updated the PR.

@sebastianbergmann any chance of this being merged? The prospect of having test cases that are silently skipped feels like quite a major issue to me.

I've checked and the build passes apart from on 5.3, which looks like a pre-existing composer / travis issue with the 4.8 branch.

@acoulton
Copy link
Contributor Author

@sebastianbergmann I was bitten by this (test case silently skipped because of a typo in the JSON, meaning a build that should have been red was green) again today. Any chance you could look at it?

Otherwise I may need to migrate from @testWith back to explicit PHP data providers as the current implementation just isn't robust enough for me.

@acoulton
Copy link
Contributor Author

@sebastianbergmann sorry to nag but 7 months later the JSON @testWith annotations are still highly unreliable - one or more test cases will be silently skipped if there are any parse errors after the first line.

It would be really good to see this merged (or another fix applied). Please let me know if I can do/provide anything further.

$dataSet = json_decode($candidateRow, true);
if (json_last_error() != JSON_ERROR_NONE) {
break;
$error = function_exists('json_last_error_msg') ? json_last_error_msg() : json_last_error();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would json_last_error_msg() not be available?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was only added in PHP 5.5.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, and PHPUnit 4.8 still needs to support that. Repressed that fact, sorry.

Copy link
Contributor Author

@acoulton acoulton Jun 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, PHP 5.5 is so long ago I had repressed it at first too :)

@sebastianbergmann
Copy link
Owner

Merged manually, thanks.

@sebastianbergmann sebastianbergmann added the type/bug Something is broken label Jun 17, 2016
@acoulton
Copy link
Contributor Author

That's great, thanks very much.

@acoulton
Copy link
Contributor Author

And sorry about the coding standards issues.

@sebastianbergmann
Copy link
Owner

No worries.

@acoulton acoulton deleted the bug/report-testwith-parse-errors branch June 19, 2016 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Something is broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants