Tests for the socket.io implementation

[megatroom]

Introduction

In the README.md it was mentioned that the next priority step would be to test the socket.io, I thought it would be a good start to contribute to my first contact with the project.
I haven't done all the tests yet, but I'd like to know if you agree with the change I made before continuing.

Description

• Add new script watch-test in package.json to run the unit tests in watching mode. This favors the practice of TDD.
• Fix profile.test.js. When running the test twice an error is thrown, to solve I put to clean the localStorage before each test.
• Remove the server initialization from app.server.js and to and put in the new file index.js.
• Create new tests in office.server.test.js for office.server.

How to test?

Now you can run with npm run watch-test 😉

Expected behavior

Everything should work as before.

[hashtegner]

@megatroom very welcome contribution! 🎆
I'd some suggestions to separate the boot file a little.

Keep coding! 🚀

[hashtegner]

I think it's a good time to / rename this file to routes.js, that receive app as dependency,
I.E:

//backend/routes.js
module.exports = (app) => { 
// build routes...

  return app
}

// backend/index.js
const app = express()
const server = require('http').Server(app);
const mountRoutes = require("./routes")
const mountSocket = require("./socket")

mountRoutes(app)
mountSocket(server)

server.listen(..)

What's up?

[megatroom]

Surely this is a great way to organize this code. Now with your feedback I feel more comfortable doing this refactoring.
I answer again when it's ready.

[megatroom]

@Alesshh I did a little different from what you suggested. I have separated in the following files:

• app.config.js - isolate the configuration constants
• app.routes.js - implementation of app routes
• app.server.js - build the server rules
• index.js
• office.factory.js - as you suggested in the other comment, I put the socket construction here
• office.socket.js - the socket logic

just changing the file name to routes.js did not make much sense because of the over responsibility that was in it. Since the design pattern was name.pattern.js, I thought it would make sense to stick to the pattern and I separated it.

[lcezermf]

Thank you for your contribution @megatroom I just made a comment, but it's not a block.

[lcezermf]

I honestly do not like too much this kind of logic inside a test, because IMHO it adds more conditions to set up the test and make it harder to reproduce and understand.

Can we simplify this logic/test to be more clear?

[megatroom]

Thanks for bring up this topic. I spent a lot of time analyzing the implementation and how I could test it.
When the backend connect, it add the current user in a default room, this trigger the "enter-room" event to the client. So, when I emit the "enter-room" from the client, I must validate the second event "enter-room" from the backend.
This leads me to raise some questions about implementation:

• Could the user connect and stay in a neutral area, like a lobby or hall? It is a region where the user is logged in, but would make it clear that he is not in any meeting.
• It is necessary trigger the "enter-room" event when the user is connected?
• If the answer is negative for the two questions above, a cleaner solution would be to pass a callback as the second parameter, through it it would be easier to validate if the response was of the event emited.

[hashtegner]

The default room when user connect is room-1 https://github.com/ResultadosDigitais/matrix/blob/master/backend/office.server.js#L3

My suggestion to test this is, test both cases, when connect and when enter-rom. This will clarify the current behavior.

 socketClient.on("connect", () => { 
       assert.equal(
            officeController.addUserInRoom.getCall(0).lastArg,
            "room-1",
          );

 socketClient.on("enter-room", () => {
          assert.equal(
            officeController.addUserInRoom.getCall(1).lastArg,
            "test-room",
          );
          done();
...

Makes sense?

[lcezermf]

I agree with @Alesshh :)

[megatroom]

Great discussion, @Alesshh @lccezinha . Besides improving the test, it's helping me to better understand the project.

The conditional is necessary because the event is triggered twice, so without the "if" the code validates only on the first trigger. I even include a comment explaining to avoid doubts of who is reading the test. If you guys prefer, I can switch to a triggered event counter instead of using the method call count. What do you think?

About the default room, I create two more tests. Analyzing the code, I saw that it uses the default room if a room is not set when connecting, so I've created a test for each of these two cases separately.

Let me know if these changes made sense.

[lcezermf]

Congratz @megatroom the code seems a lot better right now!

[hashtegner]

Move it to app.config.js, makes sense?

[megatroom]

done!

[hashtegner]

The default room when user connect is room-1 https://github.com/ResultadosDigitais/matrix/blob/master/backend/office.server.js#L3

My suggestion to test this is, test both cases, when connect and when enter-rom. This will clarify the current behavior.

 socketClient.on("connect", () => { 
       assert.equal(
            officeController.addUserInRoom.getCall(0).lastArg,
            "room-1",
          );

 socketClient.on("enter-room", () => {
          assert.equal(
            officeController.addUserInRoom.getCall(1).lastArg,
            "test-room",
          );
          done();
...

Makes sense?

[hashtegner]

WOW! Much better file separation!

particulary, i don't like the pattern file.name.js (i prefer separation by module folders), but in this case, the app is so simple that I don't see any problem with this.
One last fix and we can merge it!
You rocks!

[megatroom]

WOW! Much better file separation!

particulary, i don't like the pattern file.name.js (i prefer separation by module folders), but in this case, the app is so simple that I don't see any problem with this.
One last fix and we can merge it!
You rocks!

@Alesshh I'm with you about modular folders. Although I have seen this pattern a short time in a project and have caught my attention. But since the project was already this way, it made sense to keep the standard.

I met the project during the interview for RD, I found it very interesting and I decided to contribute, including today someone from my currently company shared this project, I even commented that I met and I'm participating with a PR. Small world 😁

And thank you for letting me participate, you can count on me for further improvements!

[lcezermf]

Just need to fix the codeclimate issue and the code seems good to me, what do you think @Alesshh ?

[megatroom]

Just need to fix the codeclimate issue and the code seems good to me, what do you think @Alesshh ?

@lccezinha the codeclimate issue is about backend/office.socket.js that I only change the file name, at the request of the @Alesshh to rearrange the files of the root.

I agree with the reported problem, but I would have to do a refactoring on the whole socket, and would run away from the purpose of this PR. If you want I can create a new PR with the purpose of refactoring the entire socket server.

[hashtegner]

I think we can handle with code climate issue next time. @juliemar, can you approves the code climate issue? I don't have permission to do this.

[juliemar]

Hi @megatroom we really appreciate your contribution to increase the project quality.. continue contributing!!!
You rocks!