Add more SASL validation and fix tests #2436
Conversation
Removes custom assert.throws(...) so that the real one from the assert package is used and fixes the SCRAM tests to reflect the updated error messages and actual checking of errors. Previously the custom assert.throws(...) was ignoring the error signature validation.
| assert.throws = function (offender) { | ||
| try { | ||
| offender() | ||
| } catch (e) { | ||
| assert.ok(e instanceof Error, 'Expected ' + offender + ' to throw instances of Error') | ||
| return | ||
| } | ||
| assert.ok(false, 'Expected ' + offender + ' to throw exception') | ||
| } | ||
|
|
There was a problem hiding this comment.
Yeah, good call. Monkey-patching assert was such a bad idea I had such a long time ago. The use of globals throughout some of the tests is also gross....need to fix up one day soonish. 🤦
There was a problem hiding this comment.
Thank you so much for this! Most of the original SASL code was contributed by not me. This is v helpful. As far as merging goes, I typically do a squash merge to keep a somewhat "one commit per PR" flow on master. (I have accidentally or for some times on purpose reasons used merge commits in the past though...I'm not super consistent.) Do you have a preference? I appreciate all the separate commits on this PR to make reviewing easier & don't want to rob you of that work if you want them all to land on master individually. It can make pin-pointing a regression in the future easier and can make the job of people who want to see what changed between versions a bit easier though if its a single squash-merge so they can kinda go "github commit page -> link to each PR which landed the change".
|
My pleasure and thanks as always for this awesome lib. Yes I like the separate commits mainly to simplify reviewing too. Especially for the incremental ones like function renames as it let you focus on the real changes in other commits. Plus it lets me know along the way what (I) broke. No preference on how it gets merged though. FYI, the additional validations break the (broken) SASL tests so it's really only a full unit as a whole anyway. |
Bunch of commits to clean up some of the SASL internals, add validation for function args, validation for data types of server responses, and some regex validation as well (e.g. base64 server responses should be valid base64). They're broken up into separate commits to make it easier to review and the final file is a bit easier to digest too.
Also removes the global
asssert.throws(...)test helper as it was overwriting the real one in the built-in asserts module. The removed one did not actually check the error signature so it would report success for unrelated errors as long as the code block threw something (which was kind of confusing while testing this out...).The only thing using that extra validation of the error signature was the SASL tests so it shouldn't break anything and all the tests pass with that change. The SASL tests have also been updated to add in some missing parameters. Previously they weren't supplying all the args needed to actually perform the tests so they'd have early or unrelated failures.