Skip to content

Conversation

@trading-peter
Copy link

Here's the first try on an implementation. It works fine in my project but I need to test if the params and credentials arguments are properly working first. I've added these to stay as compatible with nes as possible. I'm not sure how to test the filter option properly though.

Submitting the pr so you can take a look and give feedback in case would like to do something different or have other suggestions.

filter: function (path, message, options) {
return true
}
})
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test where the filter function return false? Also, you'll need to add some assertions on path, message and options.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the filter function returns false the test times out b/c the message doesn't arrive. I'm not sure how to assert this. I will add assertions for three params as soon as I got some time.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just create a different experiment where you do not call pubSubTest, but rather write actual tests for things. Also, it will require a test for promise form of the async function.

leobudima and others added 5 commits January 16, 2020 09:33
* Update to @hapi/nes 12.x and @hapi/hapi 19.x

* Update dependencies and switch tests to @hapi/lab and @hapi/code

* Update CI to Node 12 (Hapi requirement)
package.json Outdated
{
"name": "multines",
"version": "1.0.0",
"version": "2.0.1",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please restore this?

{
"name": "multines",
"version": "1.0.0",
"version": "2.0.0",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems still a bump.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants