search

noodlefrenzy / node-amqp10

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

Allow Link detach options to be overriden #254

Closed dnwe closed 8 years ago

dnwe commented 8 years ago

Note, the original code would also always acknowledge a remote detach with closed=true, regardless of what was supplied in the @detach. That has also been fixed to response with a matching value.

Fixes #249

The test_link.js unittest I added could probably be tidied up by someone more familiar with the structure of these so that it only actually validates that the correct detach frame is sent rather than asserting on the entire begin, attach, detach, end sequence. I'm still getting to grips with how to write those.

mbroadst commented 8 years ago

LGTM, thanks!

mbroadst commented 8 years ago

@dnwe in the future would you mind following commit guidelines documented here: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit ? We have automated the changelog updating and it's using that style, helps users track what's changed in a concise format. I'll go ahead and rebase for these commits that have already made it in, but just FYI in the future

dnwe commented 8 years ago

Ah sorry I hadn't spotted the commit log format.

Also note, looks like I broke Travis as I forgot to jshint after writing the unittests

dnwe commented 8 years ago

Are you ok to fixjsstyle for me or did you want me to submit another commit?

mbroadst commented 8 years ago

@dnwe yeah I'll clean it up for you. I'm actually going to sort out this code to skip the SB/EH tests if the variables aren't present so I don't accidentally merge in broken code again ;)

mbroadst commented 8 years ago

@dnwe hey so I've merged in fixes to get your tests building, as well as skipping SB tests if they aren't provisioned properly. I set the newly added link tests here (renamed to the preferred .test.js filename format, link.test.js) to be skipped as they do not presently pass. Did they work for you previously? I noticed you were using the old unit test format, using the MockServer from mock_amqp.js, did you see that there was a newly created (and perhaps more flexible) MockServer in mocks/server.js ?

If you take a look here, for example, you can see how it sets up the response sequence, and then otherwise lets you use the client as you otherwise would as a user - rather than setting up the component private API parts explicitly. Either way is totally fine as long as the tests pass, just wanted to let you know that option was there. Unfortunately the unit test fixtures are pretty hard to work with at the moment, but the hope was to grow them into something we could use more fluidly.

dnwe commented 8 years ago

@mbroadst hmm the tests were passing for me, although I did spot a further update needed to the detach promise which I've submitted as #256 and enabled the tests to confirm if they pass on Travis