search

Meteor-Community-Packages / meteor-collection-hooks

Meteor Collection Hooks
https://atmospherejs.com/matb33/collection-hooks
MIT License
657 stars 92 forks source link

find/findOne userId tests #21

Closed mizzao closed 10 years ago

mizzao commented 10 years ago

I just spent some time fixing the find/findOne userId tests (5de03e1b49734a23c613c5e97690114423009a59), and wanted to document some weird stuff in case someone wanted to look into it.

First, the original structure of the tests had some logical problems. The test.notEqual assertion was run inside the hook to check for the userId. This assumed that the hook was actually being run in the first place. If it wasn't, the test would pass tautologically. I guess this is okay if something else checks for the hook, but it seemed to be safer to not assume anything for this test if we didn't have to.

The tests are also created in the publish function to get around the asynchrony issues that @matb33 was having before. However, this results in some complaining from Tinytest when the publish function is hit again. I'm interested in seeing any creative ways to fix this problem so the test works across reloads (see below.)

Otherwise, I observed two weird issues:

  1. Other (non find/findOne) tests pass upon the first load of the Tinytest page, but not on subsequent ones. The find/findOne tests still pass. After poking around a bit, I think this is due to improper ignoring of the hooks that were added during these tests. Would be great if someone could look into it.
  2. Tinytest.add/Tinytest.addAsync doesn't seem to work inside InsecureLogin.ready() for some reason. (Test just don't appear.) See tests/find_findone_userid.js. I moved them out for now, and on my computer the login happens quickly enough for the tests to pass. However, it isn't logically correct.

Finally, do we need to add tests for userId being available in find/findOne calls from server side methods? This is way less complicated than the publish case, as the Meteor.userId() is typically already available. But just wondering...

P.S. Loving the new two-space soft tabs that you are using. Makes it much easier to contribute!

mizzao commented 10 years ago

@matb33 Actually, upon closer inspection it looks like the tests are failing on reload because the find hooks are not properly cleaning up the referenced userId. I'll let you take a look at this first before I do more digging. Just run the tests and hit reload on the page to see what happens. The tests are all in insert/update/remove.

matb33 commented 10 years ago

@mizzao First, big thanks for working on this. Apologies for not responding any sooner!

Second, I looked at your new tests (nice work) and noticed your TODO concerning the tests getting duplicated on the server on client reload. I did a simple if statement to guard against that, and it appears to be working now.

Concerning how Tinytest doesn't like us adding tests once the InsecureLogin.ready fires, I suspect it's because Tinytest's test runner expects all tests to be defined on the initial tick of the event loop. That is to say, if we define any tests asynchronously, the test runner won't find them and doesn't have any plumbing to support defining tests later on.

EDIT: I'll let you close this issue if you're satisfied with your work and my if statement

mizzao commented 10 years ago

Hi @matb33, looks good. I did notice some tests failed right when I first started Meteor, but on subsequent tests (either manual reload or hot code reload) everything worked, so it's probably not worth pursuing that. And it's not because of the other error I conjectured, so that's good.

I think the adding tests problem is a little more nuanced than that. For example in that same file we add the server tests in the publish function and it works just fine. Maybe it's what you said, but only on the client side (server supports adding tests whenever.) I would say it's probably a bug with Tinytest but something we shall just work around for now because it's not the future of Meteor testing anyway.

I've been using InsecureLogin for tests in some of my other packages as well. Really neat code. Would be awesome if it supported both logging in and logging out during tests instead of just once at the beginning - I might be adding that for something else soon, but loathe to spend so much time investing in a testing methodology that is not really future proof.