search

strongloop / strong-remoting

Communicate between objects in servers, mobile apps, and other servers.
www.strongloop.com
Other
105 stars 93 forks source link

Test rest coercion #304

Closed bajtos closed 8 years ago

bajtos commented 8 years ago

An integration test suite describing and verifying argument coercion in REST.

1230 passing (3s)

(see https://github.com/strongloop-internal/scrum-loopback/issues/706#issuecomment-217371058 for more information)

In this pull request, I am describing the current behaviour in the master branch. My intention is to run the test suite against different strong-remoting versions to find differences, I'll post results later.

In order to discover all breaking changes and document the new behaviour, we should write an extensive integration test suite that can be run against different versions of strong-remoting. Here is an initial draft for inspiration: https://gist.github.com/bajtos/b85e53a79ba06671e08d2013acccc611 Versions to run the suite against: v2.16.0, v2.16.1, latest 2.x, master (3.0-alpha). This commit may have introduced breaking change in v2.16.1: strongloop/strong-remoting@34b0665

Connect to strongloop-internal/scrum-loopback#884 Connect to strongloop-internal/scrum-loopback#913

@ritch @STRML please take a look

bajtos commented 8 years ago

See also https://github.com/strongloop-internal/scrum-loopback/issues/706#issuecomment-217371058

STRML commented 8 years ago

Looks good so far, I agree with the idea of testing this more thoroughly. And it does indeed appear 34b0665 broke some backcompat, but you could argue previous behavior was a bug in the first place. Still worthwhile for cleaning up for good in 3.0.

STRML commented 8 years ago

See also https://github.com/strongloop/strong-remoting/pull/265 and especially https://github.com/strongloop/strong-remoting/pull/265#issuecomment-164527886 as handling this code was a comedy of errors - somebody inside strongloop force-pushed it out without any notice, then only half the PR was re-merged and released without review, causing significant breakage as the code was incomplete (it was two commits and should not have been merged incomplete).

bajtos commented 8 years ago

Done. @STRML @ritch PTAL

bajtos commented 8 years ago

I have cleaned up the code a lot, restructured test cases and added comments to make it more clear what is being tested. The patch is ready for another round of code review.

@STRML @ritch PTAL.

bajtos commented 8 years ago

The high-level overview of changes between 2.16.x versions can be found here: https://gist.github.com/bajtos/8afaa9d1ed7d7b410c76ec2095a8a992

ritch commented 8 years ago

LGTM

I don't see anything that should change. But there is quite a bit I may be overlooking still. I think the size of this test suite points to a separate module making sense for coercion.

The documentation in these tests would also be helpful to everyone. I think we should create a section about coercion in the readme that lists out the nuances of coercion. Which can be done outside of this PR.

Amir-61 commented 8 years ago

@slnode test please

Amir-61 commented 8 years ago

I did not see any problem either. The patch LGTM.

davidcheung commented 8 years ago

LGTM too should be really helpful for us to prevent regression on a much more detailed level :+1:

bajtos commented 8 years ago

@ritch @Amir-61 @davidcheung thank you for the review!

The documentation in these tests would also be helpful to everyone. I think we should create a section about coercion in the readme that lists out the nuances of coercion. Which can be done outside of this PR.

I filled a new task for that - https://github.com/strongloop/strong-remoting/issues/313, I think it will be best to document the new behaviour after we make the intended changes/fixes.