search

noodlefrenzy / node-amqp10

amqp10 is a promise-based, AMQP 1.0 compliant node.js client
MIT License
134 stars 56 forks source link

update tests to support different runtime versions #329

Closed bengreenier closed 6 years ago

bengreenier commented 7 years ago

A couple of tests fail on the master branch, as a result of downlevel changes in node core. Specifically, node>=5.7.0 adds encodeAuth() (see changelog for which release it landed in - 5.7.0) which inteligently doesn't encode : as a uri component when creating a Url. Rather than make any changes to our library to normalize this behavior across versions (which feels like a bad fix, since this behavior is consistent with core) we added support conditionally executing tests based on a version check against the current runtime.

We apply this new version checking behavior to the failing test, and provide an additional new test - the result is a version for node < 5.7.0 and a different version for node >= 5.7.0, to test both expected behaviors on the respective runtimes.

This change is Reviewable

bengreenier commented 7 years ago

hmm, this is triggering a build failure:

Creating home directory `/var/run/qpid' ...
/etc/init.d/qpidd: 52: /etc/init.d/qpidd: source: not found
user had insufficient privilege
invoke-rc.d: initscript qpidd, action "start" failed.
dpkg: error processing package qpidd (--configure):

I'll continue to look into it - i suspect something about how the qpid deb installs itself has changed since the last official build...

mbroadst commented 7 years ago

@bengreenier thanks for the contribution. I don't agree with the changes to .travis.yml (not sure why they are necessary), and I think perhaps the solution is a little too heavy handed. afaict, the change introduced in #319 is what broke this, and it was only broken for ancient versions of node (0.10/0.12), which are only presently still supported because of some existing MS customers.

It should be enough to simply modify this code to accommodate both cases (maybe do a string check against process.version or something). Your solution would simply not run the test against older versions of node, and thus continue to (silently) break on those versions.

bengreenier commented 7 years ago

@mbroadst anytime - been talking to @noodlefrenzy about getting more involved here. 😄

The .travis.yml changes will go away, was just trying to figure out why the build doesn't work. I abstracted the process.version string check into a test helper because i thought this might come up again and that solution would be easier to re-purpose. I'm not just doing a string compare because it seems more brittle than pulling in semver (since it's just a devDep, i figured there's little impact).

Would you prefer less complexity (like an inline process.version check) as opposed to this approach?

FWIW, my current approach isn't just a skip for that test, it modifies the existing to only run on >=5.7.0 and adds a new one that runs on <5.7.0 - since they're effectively testing different behaviors.

mbroadst commented 7 years ago

@bengreenier oh I see, sorry I missed the second test. I'm still surprised the existing code in policy.js isn't broken for older versions of node? Otherwise, yeah, LGTM once the travis.yml is reverted

bengreenier commented 7 years ago

@mbroadst seems like the policy.js code works because the issue only impacts the url.href field, not url.auth - interestingly enough the change was in how formatting of url.auth is done into the url.href field, as i understand it.

sounds good, i'll revert that stuff - any idea why the ci build fails? i was seeing the failure with and without my travis changes.

mbroadst commented 7 years ago

@bengreenier I'd have to see the logs for when you revert travis, right now it looks like permissions error with installing qpid - but that shouldn't be the case with the old config.

bengreenier commented 7 years ago

@mbroadst sure, i'll get that reverted by EOD.

Though in the meantime, the first PR build (b899f2b) had no .travis.yml changes and still had that issue - https://travis-ci.org/noodlefrenzy/node-amqp10/builds/259141707

though If you'd prefer to wait til i revert and see if it repros that makes sense as well.

bengreenier commented 7 years ago

removed those travis changes entirely, lets see if the build clears.

mbroadst commented 7 years ago

@bengreenier looks like maybe the qpid maintainers screwed up the 1.36 release - unfortunately it will take me a bit to get to that. In the meantime you could maybe shoot an email over to iboverma@redhat.com indicating there's a problem with their ubuntu packaging (perhaps try to install it locally on an ubuntu server running trusty first?). Sorry for the trouble!

bengreenier commented 7 years ago

@mbroadst no worries, I'll investigate and ping them - will update this pr as needed.