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

Fail JSON tests that return object where array is required #4622

Merged

Conversation

Projects
None yet
4 participants
@nbrady-techempower
Copy link
Member

commented Apr 8, 2019

We should be failing tests that return a single object when an array is required. It's almost never the right number of elements and it's less bytes than we require.

Resolve #4612

@joanhey

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

I think that is correct now.
But some frameworks fail now.

I could change the php framework in a moment.
The rest of frameworks that fail, will need to change the code too.

@zloster

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

There seems to be a false positive case for the new checks:
https://travis-ci.org/TechEmpower/FrameworkBenchmarks/jobs/517302629#L6039-L6064
ringojs manages to return 501 objects in the right case.

@nbrady-techempower

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

@zloster I think this is ok? We haven't changed warning on 501+ results returned and after debugging it locally, it looks like it is indeed returning an array of 501 properly formed objects. So that warning is fine.

@zloster

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

My point was that maybe the failing rule is too strict. Not all frameworks have different implementations for single and multiple queries. With the current checks they will fail the verification.

@nbrady-techempower

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

Ah, I see what you're saying. I'm not sure I'm worried about that. Looking at travis, there certainly are a few, but I would rather those be fixed. I think the closer we get to parity/strictness of expected return results, the better. But if others don't feel that way, I'm fine to hold off on this.

@msmith-techempower

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

I agree with Nate; let's get a bit more draconian.

@joanhey

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

Sorry but you don't need 2 different implementations.

I changed the php activerecords with 1.
https://github.com/TechEmpower/FrameworkBenchmarks/pull/4606/files

@joanhey

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

Without this change, you can see things like that.
https://www.techempower.com/benchmarks/#section=test&runid=8ab47a8b-0b0d-43f7-ad12-1e96b72ff42d&hw=ph&test=query

Check the php-ngx req/s.

@zloster

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

@nbrady-techempower @msmith-techempower It's your call.
Maybe before going draconian to consider another option. Reordering the checks:
If ?queries=501 returns 500+ array of objects than issue warning. And don't force failing for the cases requiring result in the form of one element array. Because the check will catch if the framework is not returning enough data.

@joanhey Yes, it is not required to have two implementations. IMO changing the current code for given framework or implementing two separate handlers will be almost equal as effort.

@joanhey

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

I think that would fail in both situations.

WARN for http://tfb-server:8080/db?queries=501
     JSON array length of 501 != expected length of 500

And this one will be a fail too, not a warning.

The tests help to create better code.
If you are sending 100 or 502 when need to be 500.
Only help the programmer that create the code.

@nbrady-techempower

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

After some review, I think we're going to merge this in. It also doesn't require a rule change. The rules for multiple queries state:

Each World object must be added to a list or array.

I think for the purpose of benchmarking and adhering to the rules, this change makes sense.

I also agree with @joanhey that more than 500 elements should probably also be a failure. We put these rules in place to create some branching logic (though not spectacular in this case) to test the performance under heavy load. I'm not sure that returning 501 elements is a benefit vs doing a logic check and returning 500, but it's another one of those things where ensuring parity means we can focus more on benchmarking and less on who's skirting this rule & that rule. With this one though, it's more likely that we're pointing out a problem for which the tester meant to correct.

@nbrady-techempower nbrady-techempower merged commit c47a11f into TechEmpower:master Apr 9, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@zloster

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

IMO I haven't explained it well enough:
I don't mind having stricter verification of the rules. I had considerations about the execution of the change. In my mind:

  1. Change verification for the multiple queries to fail if there is a single result for the request "?queries=501" - this was the original problem in the linked issue. Merge this code;
  2. Announce (by pinning the issue and/or mailing the list) the planned stricter checks;
  3. Wait 1 or 2 months and than implement the change. This gives an opportunity for some of the participants to fix the implementations.

The result should be "everyone is happy and there are stricter checks" but IMO the transition is smoother.

@nbrady-techempower

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

@zloster I see. I think I misunderstood your original issue. Well, the ship on this has sailed. There aren't many tests that fail because of this. @joahney grabbed all the php ones. I can tackle a couple this week and that's pretty much it. At least the rules always said must so we're not getting any stricter with the rules, we're getting stricter with the enforcement. I'll make a pinned issue this week describing some of the changes that we've made, like #4291

@zloster

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

@nbrady-techempower So, everything is clear.
I'm preparing a change for the Java/restexpress.

nbrady-techempower added a commit that referenced this pull request Apr 9, 2019

Make Java/RestExpress to conform to #4622 (#4632)
* Add check to distinguish between db and multiple queries

* Handle non-numeric parameter value

* Add separate implementations for db and query tests for MySQL

* Disable SSL for MySQL connections

* Add separate handlers for db and queries for MongoDB

nbrady-techempower added a commit that referenced this pull request Apr 22, 2019

[ci fw-only Java/play1] Play1 to conform to #4622 (#4685)
* Add separate db and query endpoints

* Use correct folder name in the Docker file. Add README.md clarification

daviddenton added a commit to http4k/FrameworkBenchmarks that referenced this pull request Jun 1, 2019

daviddenton added a commit to http4k/FrameworkBenchmarks that referenced this pull request Jun 1, 2019

Make Java/RestExpress to conform to TechEmpower#4622 (TechEmpower#4632)
* Add check to distinguish between db and multiple queries

* Handle non-numeric parameter value

* Add separate implementations for db and query tests for MySQL

* Disable SSL for MySQL connections

* Add separate handlers for db and queries for MongoDB

daviddenton added a commit to http4k/FrameworkBenchmarks that referenced this pull request Jun 1, 2019

[ci fw-only Java/play1] Play1 to conform to TechEmpower#4622 (TechEmp…
…ower#4685)

* Add separate db and query endpoints

* Use correct folder name in the Docker file. Add README.md clarification
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.