Major Fixing and Refactoring tests

[lirantal]

1. Refactoring variables usage through-out the tests
2. Fixing correct error handler tests were previously these would report a false positive isue
3. Fixing recent unit tests to be added as part of the main save method suite
4. Fixing an issue with the tests which didn't clean the user1 entry in the db and so tests following it would fail regardless of the validation
5. Fixing one test to actually be valid use case

[rhutchison]

if err did exist, it would never hit this case?

[lirantal]

If err exists then it means the test failed and there's no user object that needs removal.
If err doesn't exist then it means the test succeeded and we need to

1. remove the user that we just added (to clean up the db for another test)
2. assert that err doesn't exist

[rhutchison]

Correct, but I read is as line 447 if (!err) { (err does not exist) would never get to line 449 should.not.exist(err);

[lirantal]

If the code enters into the code block of 447 then it means that _user1 object was created successfully, and so what we do is take care of removing _user1 object from the db for further tests, and then we assert that (err) object shouldn't exist which is indeed correct because that's the reason we entered the if block in first place.

[mleanos]

@lirantal I see the same thing as @rhutchison with line 449

should.not.exist(err); will always pass, since it couldn't have gotten into this block otherwise from line 447. I think you could move the assertion at line 449 to 447; right after the _user1.save

I notice you're implementing this logic throughout these tests. Perhaps, I'm not understanding the intent of putting the asserts inside the blocks this way.

[mleanos]

@lirantal I've tested this, and confirmed the tests are working as expected. Only thing is the line comment I left. Other than that, everything LGTM.

[lirantal]

Updated the tests but just to clarify, it was more of a refactoring to not call the assertion from two places, but in the previous version it was just fine to put it in both the if and the else blocks.

Anyway, updated.

[mleanos]

@lirantal Starting with this test, the else block is checking that should.not.exist(err) at line 360. This assert will never pass if it makes it to this line, since err existing is how the code will enter this else block.

[lirantal]

I'm confused, you've commented on the outdated code so I'm not sure which part you are referring to exactly.

[lirantal]

@mleanos assuming you were referring to the new code added on line 348, try and change it as follows:

    it('should allow single quote characters in email address - "abc\'def@abc.com"', function (done) {
      var _user1 = new User(user1);

      _user1.email = "abc\'def@abc.com";
      _user1.save(function (err) {
        err = {};
        if (!err) {
          _user1.remove(function (err_remove) {
            console.log('if');
            should.not.exist(err);
            should.not.exist(err_remove);
            done();
          });
        } else {
          console.log('else');
          should.not.exist(err);
          done();
        }
      });
    });

and then just play with the err = {}; line and change it to null and back to an empty object to see how the tests behave.

The reason that should.not.exist(err) assertion is in the else block on 360 is that if the _user1.save operation fails and produces an err object then the assertion there on line 360 will fail the test because it expects err not to exist but it does.

Hopefully I explained it ok

[mleanos]

@lirantal Thanks for clarifying that. I wasn't looking at it in that way. You're forcing the assertion to fail, since you have the if else block. I hadn't realized what your meant by "Major fixing.." until the other day when I was writing some tests.

To make sure I understand what your refactoring is accomplishing & that I understand it.. let me ask a question that may be a LDO :) .. So with these changes, you're trying to get all the tests to continue to function properly (pass/fail) even when previous tests fail? This is something I ran into with how the tests are setup currently.

[mleanos]

@lirantal I understand that this PR was meant more for refactoring certain aspects of the tests. I'm still learning how to work with these testing frameworks, so I'm just trying to understand :)

[lirantal]

@mleanos sure, you're welcome to further review.
We need more tests and more methods of tests (i.e: also testing core items, and also using supertest for HTTP API testing on the users object and other core resources)

[mleanos]

@lirantal I'm currently working with @codydaig & @bastianwegge to get server side test coverage of the Socket IO server... not easy stuff :) If you have any insight, or have already started working on that, I would love to hear about it.

[bastianwegge]

@lirantal To point out what's the problem: We're having trouble getting the sessionId within the socket.io.js configuration part, since we're not sure how to pass cookie/session-Variables into socket.io-client (what seems to be usable for testing socket.io server part)

[lirantal]

Guys I'll go ahead and merge the changes here as they are considerable and will later only cause more headache to rebase with conflicts.

Let's take the http testing to another thread.
It's on my todo list ever since we found the bug there with the session key name change but I didn't yet get to write anything.

[codydaig]

@lirantal SGTM

[mleanos]

@lirantal SGTM too :)

[lirantal]

Great. Merging.