Configurable socket namespace per emit action #125

menzow · 2016-11-30T16:22:54Z

This change allows socket.IO namespaces to be configured by adding a namespace attribute to the emit actions.

More info: #124

hassy · 2016-12-01T14:03:00Z

Can you add a test case for the new functionality please?

An existing socket.io test seems to be broken as well.

menzow · 2016-12-01T14:15:20Z

Hey @hassy,

Thanks for your feedback, I fixed the failing test.
Where do you want me to add the additional tests?

I guess a new test scenario should be added here: artillery-core/test/scripts called namespaces_socketio.json.

Any other tests that need to be created?

hassy · 2016-12-01T14:27:46Z

@menzow Cool, cheers. Yeah a simple script using namespaces will do, perhaps using two different namespaces and verifying the messages that come back. The test Socket.io servers[1] will need to be updated to send something back (same value) on different namespaces, and perhaps to print the number of clients/messages in different namespaces as well to help debugging.

https://github.com/shoreditch-ops/artillery-core/blob/master/test/targets/simple_socketio.js and https://github.com/shoreditch-ops/artillery-core/blob/master/test/targets/express_socketio.js

…ion is defined

menzow · 2016-12-03T03:36:36Z

Hey @hassy,

Added tests for namespaces and fixed the errors. Please review it.

Edit: I did add the namespace tests to the simple_socketio.js server instance. Not sure what the guidelines are on that. I could create a separate server instance that handles the namespace tests

hassy · 2016-12-05T16:38:12Z

Cool, thanks @menzow

hassy · 2016-12-06T14:55:49Z

lib/engine_socketio.js

    // Only process emit requests; delegate the rest to the HTTP engine (or think utility)
    if (requestSpec.think) {
-      return engineUtil.createThink(requestSpec, _.get(self.config, 'defaults.think', {}));


We need to preserve the second argument to createThink, otherwise default think settings won't be applied.

You're right. I've put it back.

hassy · 2016-12-06T15:00:47Z

lib/engine_socketio.js

@@ -118,10 +118,10 @@ SocketIoEngine.prototype.step = function (requestSpec, ee) {
    return engineUtil.createLoopWithCount(requestSpec.count || -1, steps);
  }

-  let f = function(context, callback) {
+  let f = function(context, callback, socketio) {


Is there a particular reason we are passing socketio as an extra argument here, rather than keeping it attached to context? That function's signature should really follow that of async#waterfall task functions since that's where f is used. This would avoid us needing to wrap f in preStep as well.

Ah you're right, I overlooked that part. Please see next commit.

hassy · 2016-12-06T15:01:33Z

@menzow I added a couple of comments inline. Thank you for your work.

menzow

Processed feedback.

menzow · 2016-12-08T09:42:17Z

lib/engine_socketio.js

    // Only process emit requests; delegate the rest to the HTTP engine (or think utility)
    if (requestSpec.think) {
-      return engineUtil.createThink(requestSpec, _.get(self.config, 'defaults.think', {}));


You're right. I've put it back.

menzow · 2016-12-08T09:43:05Z

lib/engine_socketio.js

@@ -118,10 +118,10 @@ SocketIoEngine.prototype.step = function (requestSpec, ee) {
    return engineUtil.createLoopWithCount(requestSpec.count || -1, steps);
  }

-  let f = function(context, callback) {
+  let f = function(context, callback, socketio) {


Ah you're right, I overlooked that part. Please see next commit.

…ameter from step:f() function for better compatibility with async#waterfall

menzow · 2016-12-08T11:44:20Z

lib/engine_socketio.js

-    if (!(requestSpec.emit && requestSpec.emit.channel)) {
-      ee.emit('error', 'invalid arguments');
+    if (!(requestSpec.emit && requestSpec.emit.channel && socketio)) {
+      return ee.emit('error', 'invalid arguments');


@hassy, I added socketio to be required here either. And I return after the error emit. Continuing execution without these variables seems unnecessary / problematic.

menzow · 2016-12-08T11:45:42Z

lib/engine_socketio.js

-SocketIoEngine.prototype.getSocket = function(namespace, context, cb) {
-  context.namespaces = context.namespaces || {};
+SocketIoEngine.prototype.loadContextSocket = function(namespace, context, cb) {
+  context.sockets = context.sockets || {};


Renamed context.namespaces to context.sockets. Naming seemed more explanatory.

menzow · 2016-12-08T11:50:18Z

lib/engine_socketio.js

@@ -118,20 +118,21 @@ SocketIoEngine.prototype.step = function (requestSpec, ee) {
    return engineUtil.createLoopWithCount(requestSpec.count || -1, steps);
  }

-  let f = function(context, callback, socketio) {
+  let f = function(context, callback) {


Removed socketio parameter.

Socket is now taken from context.sockets[requestSpec.emit.namespace] at line 132.

menzow · 2016-12-08T11:55:14Z

lib/engine_socketio.js

@@ -180,52 +181,55 @@ SocketIoEngine.prototype.step = function (requestSpec, ee) {
  };

  function preStep(context, callback){


preStep behaves as a decorator for function f. It ensures that the socket for the emit namespace is loaded before executing f.

Omitting it would add unnecessary complexity inside the f function. Separation keeps everything a bit more in line with the single responsibility principle. I ensured that the invocation parameters of preStep and f are the same.

hassy · 2016-12-08T13:31:21Z

Cool, nice work @menzow 👍

hassy · 2016-12-08T13:32:33Z

Would you mind squashing all of your commits here into one? Just one last thing before I merge.

menzow · 2016-12-08T13:59:21Z

Hey @hassy ,
Awesome that it's going to be merged!

Created a new PR #129 with the squashed commits, however I'm not familiar with the technique. Hope that one is correct.

hassy · 2016-12-08T14:09:25Z

Cool. You could've done this instead to avoid needing to create a new PR:

# in your fork:
git rebase -i HEAD~6
# change "pick" to "s" in the editor for all commits but the first one, save and exit
# in the next editor window, rewrite the commit message
# then:
git push -f origin master
# that would've updated this PR directly instead

hassy · 2016-12-08T14:10:22Z

Merged the other PR. Cheers @menzow

menzow · 2016-12-08T14:16:46Z

Thanks for merging! Helps out my test case a lot. I'll add a new topic in the docs explaining how to use namespaces so it may help out other users.

One unrelated question:
Since I already pushed my changes, doesn't squashing the last 6 commits on the same branch mess up the commit history / commit references in github?

hassy · 2016-12-08T14:19:42Z

Github keeps all commits, even the ones that have been over-written with a force-push, so it's still possible to see the original discussion, it's just marked as "outdated".

menzow · 2016-12-08T14:22:27Z

@hassy , thanks for the info 😃. Cheers.

Menzo Wijmenga added 2 commits November 30, 2016 09:00

Allow socket.io namespaces to be configured in emit action

a02ec88

Move closeContextSockets method into SocketIoEngine class

2bed27e

menzow mentioned this pull request Nov 30, 2016

Make namespaces configureable per emit action #124

Closed

Menzo Wijmenga added 3 commits December 3, 2016 09:58

Fixed invalid config scope, and added test case for socket.io namespaces

87578c1

Fixed engine_socketio.js js_lint compliance

e4959b9

Only run preStep action to load a namespaced socket when the emit act…

cfc7d14

…ion is defined

hassy reviewed Dec 6, 2016

View reviewed changes

menzow commented Dec 8, 2016

View reviewed changes

Fixed second parameter for createThink. Removed optional socketio par…

5974ede

…ameter from step:f() function for better compatibility with async#waterfall

menzow commented Dec 8, 2016

View reviewed changes

menzow mentioned this pull request Dec 8, 2016

(Squashed) Allow socket.io namespaces to be configured in emit action #129

Merged

hassy closed this Dec 8, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configurable socket namespace per emit action #125

Configurable socket namespace per emit action #125

menzow commented Nov 30, 2016

hassy commented Dec 1, 2016

menzow commented Dec 1, 2016 •

edited

Loading

hassy commented Dec 1, 2016

menzow commented Dec 3, 2016 •

edited

Loading

hassy commented Dec 5, 2016

hassy Dec 6, 2016

menzow Dec 8, 2016

hassy Dec 6, 2016

menzow Dec 8, 2016 •

edited

Loading

hassy commented Dec 6, 2016

menzow left a comment

menzow Dec 8, 2016

menzow Dec 8, 2016 •

edited

Loading

menzow Dec 8, 2016 •

edited

Loading

menzow Dec 8, 2016 •

edited

Loading

menzow Dec 8, 2016

menzow Dec 8, 2016 •

edited

Loading

hassy commented Dec 8, 2016

hassy commented Dec 8, 2016

menzow commented Dec 8, 2016 •

edited

Loading

hassy commented Dec 8, 2016

hassy commented Dec 8, 2016

menzow commented Dec 8, 2016

hassy commented Dec 8, 2016

menzow commented Dec 8, 2016

		@@ -180,52 +181,55 @@ SocketIoEngine.prototype.step = function (requestSpec, ee) {
		};

		function preStep(context, callback){

Configurable socket namespace per emit action #125

Configurable socket namespace per emit action #125

Conversation

menzow commented Nov 30, 2016

hassy commented Dec 1, 2016

menzow commented Dec 1, 2016 • edited Loading

hassy commented Dec 1, 2016

menzow commented Dec 3, 2016 • edited Loading

hassy commented Dec 5, 2016

hassy Dec 6, 2016

Choose a reason for hiding this comment

menzow Dec 8, 2016

Choose a reason for hiding this comment

hassy Dec 6, 2016

Choose a reason for hiding this comment

menzow Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

hassy commented Dec 6, 2016

menzow left a comment

Choose a reason for hiding this comment

menzow Dec 8, 2016

Choose a reason for hiding this comment

menzow Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

menzow Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

menzow Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

menzow Dec 8, 2016

Choose a reason for hiding this comment

menzow Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

hassy commented Dec 8, 2016

hassy commented Dec 8, 2016

menzow commented Dec 8, 2016 • edited Loading

hassy commented Dec 8, 2016

hassy commented Dec 8, 2016

menzow commented Dec 8, 2016

hassy commented Dec 8, 2016

menzow commented Dec 8, 2016

menzow commented Dec 1, 2016 •

edited

Loading

menzow commented Dec 3, 2016 •

edited

Loading

menzow Dec 8, 2016 •

edited

Loading

menzow Dec 8, 2016 •

edited

Loading

menzow Dec 8, 2016 •

edited

Loading

menzow Dec 8, 2016 •

edited

Loading

menzow Dec 8, 2016 •

edited

Loading

menzow commented Dec 8, 2016 •

edited

Loading