Is testing the todos way primetime ready?

[rhettlivingston]

I followed the model represented in lists/methods.js and lists/lists.tests.js in writing my own application and tests. I like the model and the potential it represents. However,

Exceptions in the ValidatedMethod._execute calls my tests make are often not being caught by pm mocha. The main one that repeatedly gave me trouble was the "throw error;" in line 425 in collection2.js. That seems to throw on just about anything going wrong in inserts or updates to the collection. And most of the things going wrong only effected testing.

When this exception is thrown, the test simply never shows in the console (even when the _execute call to the method is wrapped in assert.Throws or assert.doesNotThrow!). You can see that not all tests have run by the percentage completion not being 100%, but you have no guidance as to what test(s) are failing except by comparing the list of passed tests to your .tests. file and identifying the missing one.

These exceptions also do not show in any console!

I ended up spending a lot of time in the server-side debugger (which seriously needs an update by the way... hope that is coming soon when meteor's node.js update enables it).

My initial failures were due to this.userId not being available to defaultValue and autoValue functions in my schema. It is available when not running under pm mocha because of line 170 in collection2.js that uses Meteor.userId to set the context. I suspect referencing this.userId in the schema is not uncommon. Such things are why ValidatedMethod was created, but the userId provided to the _execute interface does not automatically get through to collection2 (of course).

In short, exceptions not being caught and in fact being hidden (not showing in consoles either) is a big problem when working at a stage of development where exceptions are expected. Also, the dependency on Meteor.userId() for normal operation inside both collections2-core and SimpleSchema (it has a reference also), makes calls to a non-stubbed List().update dangerous in a test environment,,, but I want to make them anyway because stubbing out the collection just goes too far. I expect just that little bit of "integration" testing in my unit tests as I think the creators of this todos example expected.

I see several possibilities for improvement here and don't know what the correct approach(es) is/are.

1. Should todos/methods.js be changed to pass a validationContext defining userId to collections2 when invoking Lists.insert or List.update? Downside is that we're changing code to accomodate testing.
2. Should a bug report be made to pm mocha? Or is hiding this exception an expected behavior?
3. Should a bug report be made to collections2-core for how the exception is being thrown on line 425? Or is that perfectly valid? I did see that similar behavior in SimpleSchema was changed to help support ValidatedMethod.

Or is all of what I've experienced actually what we want developers to deal with? I guess it was a learning experience after all.

[tmeasday]

Hi @rhettlivingston, thanks for your detailed comments. I think the headline here is that this stuff is all pretty new and there's a fair bit of room for improvement. Let's make it happen!

1. Should todos/methods.js be changed to pass a validationContext defining userId to collections2 when invoking Lists.insert or List.update? Downside is that we're changing code to accomodate testing.

I think probably the correct answer is to stub out Meteor.userId also (or the collection insert itself?).

1. Should a bug report be made to pm mocha? Or is hiding this exception an expected behavior?

Yes, definitely!

1. Should a bug report be made to collections2-core for how the exception is being thrown on line 425? Or is that perfectly valid? I did see that similar behavior in SimpleSchema was changed to help support ValidatedMethod.

I think throwing a ValidationError when validation fails is what we want C2/SS to do. Is that what's going on in this case?

I suspect issue #2 here is causing a lot of unnecessary pain here.

[rhettlivingston]

(or the collection insert itself?).

I finally realized consciously why I feel this is bad... there would then be no way to unit test my code that is in autoValue specifications in my schema. Schema specifications themselves are code that should be unit tested.

I think throwing a ValidationError when validation fails is what we want C2/SS to do. Is that what's going on in this case?

As the problems were often in my autoValue (or defaultValue which is really the same thing) code that was being run during the doClean in collection2, I don't think ValidationErrors were being thrown.

The fact the problems were occurring in autoValue specs was also why I was encountering them in the collection2 inserts/updates instead of ValidatedMethod's validation. SimpleSchema's validator function does not clean by default. So the validation on entry into ValidatedMethod._execute was passing. That is part of what led me to publish rlivingston:simple-schema-mixin for ValidatedMethod.

I suspect issue #2 here is causing a lot of unnecessary pain here.

Agree. I'll report it.

I think probably the correct answer is to sub out Meteor.userId

I think stubbing Meteor.userId is probably something that should be done. For example, if todos had been defaulted to private, a "defaultValue: this.userId;" in the schema would have been appropriate and wouldn't have worked without stubbing Meteor.userId to report the userId passed into _execute.

Since the context userId passed to _execute often changes and I'd want Meteor.userId() to just follow that (I dislike doing things more than once in code), I'm debating on the right approach to stubbing Meteor.userId().

Instinct tells me that the purpose of ValidatedMethod's _execute is not being met unless it sets Meteor.userId() whenever a caller supplies a context that specifies a userId. That one stub may be more important than any other. But, it would have to be very careful to only provide it's own stub if there is no Meteor.userId() already in the environment...

would such a fix in ValidatedMethod's _execute be reasonably doable? Or should the test really be doing the stub itself even though _execute is there to help prevent the need?

[tmeasday]

I think in general getting away from code that calls Meteor.userId() is good, but there's not much we can do about it in the SS/C2 I guess.

I'm not sure ValidatedMethod should be messing with Meteor.userId like that, it might be very surprising to people (I think _execute is called in normal code too?).

Maybe there should be a ValidatedMethod.executeAsUserId() or something that more explicitly is a test method that should not be called in other contexts? I think we'd take a PR for that

[rhettlivingston]

It took me a while to duplicate my issue in todos, but I finally figured it out. Initially, I added the default but forgot to remove the optional : true, so the default didn't get run. I was really scratching my head for a while and starting to believe that Factory (which I don't use) did a lot more than I thought. Then I backed up, looked again, and thought about how silly it is to have an optional defaultValue. That too, probably should be throwing an error in SimpleSchema.

Anyway, changing

userId: { type: String, regEx: SimpleSchema.RegEx.Id, optional: true },

at line 40 in lists.js to

userId: { type: String, regEx: SimpleSchema.RegEx.Id, defaultValue: this.userId },

causes the server side test to hang up with no tests (pass or fail) or errors being reported anywhere. If you get in the debugger and look at what is actually happening, you'll find all of the tests actually running but throwing an exception again and again. The exception doesn't even appear in the debugger, so you just have to spend the time to trace the code until you hit the line that just never hits another breakpoint. Painful.

I'll now put in the bug report to pm mocha giving this as an example (though it is majorly contrived since it breaks the app).

I'm not sure ValidatedMethod should be messing with Meteor.userId like that, it might be very surprising to people (I think _execute is called in normal code too?)

Perhaps it is being called in normal code by now because a lot of things could be put into the "context" field it provides and there is nothing to prevent calling from normal code. However, the ValidatedMethod README.md is very targeted in its example and only documentation stating

Call this from your test code to simulate calling a method on behalf of a particular user.

And it just isn't achieving the effect of "calling a method on behalf of a particular user". I do understand from some of the issues I've read that the solution involving duplicating what DDP(?) does is a bit hairy and that the whole magical Meteor.userId() thing may need to go away, but it is the promise of the packages README that this will simulate calling a method on behalf of a particular user.

I might consider tackling something for a PR, but it is a stretch goal for a newbie (though an old newbie with vast experience in other coding worlds :-) )

[tmeasday]

I think you've misintepreted what is meant there. It doesn't mean that's the only way that _execute gets called, only that's the only way you should all it. It gets called by the method directly:

https://github.com/meteor/validated-method/blob/master/validated-method.js#L54

[rhettlivingston]

I see. I'll play with it some to more fully understand.

[rhettlivingston]

Hmmm. Just realized that in the case of "todos" as opposed to my app, the line 40 change above actually causes the Factory.create calls to fail with the same behavior I've seen in ValidatedMethods since Factory invokes the collection2.insert method.

But, the true entry point of the problem is earlier. I had one of those "duh" moments when I realized that, though SimpleSchema turns defaultValue clauses into autoValue functions, there is a world of difference between the following two lines even though the function I put in the autoValue almost exactly matches the one used internally for defaultValue:

defaultValue: this.userId autoValue: function () { if (this.operator === null && !this.isSet) return this.userId; },

In the case of the defaultValue, "this.userId" is evaluated during the call to the SimpleSchema constructor and its return value is stored for use in the function. In the case of the autoValue, "this.userId" is evaluated during the collection mutation operation.

That has a profound effect of course on whether or not the extendAutoValueContext in the insert statements (as shown below) can help.

self.collections[collectionName].insert(doc, { extendAutoValueContext: { userId: "some userId source" } });

It could also lead to really hard to find problems when variables or functions referenced in defaultValues change between SimpleSchema instantiation and usage in mutators. I wonder how many undiscovered issues are out there due to this behavior. I guess it should have been obvious to me, but my belief was that the code I provided was being plugged into the autoValue, not its evaluation. Maybe I should ask that the SimpleSchema readme point out this difference.

So, a) changing ValidatedMethod cannot completely address the issue of supporting code references in schemas. b) changing both VM and Factory still falls short unless you also make sure the right "this" is in place during any SimpleSchema instantiations. c) and even if you do all of that, during testing you often want to simulate multiple userIds inserting data, so you'd better be careful not to share simpleSchema's instantiated during the simulation of different userIds. Not every "List" is created equally.

This has been a little eye-opener to me of just how pervasive the magical Meteor.userId() issue can be in testing.

But it is also a reminder that without that, I guess we'd be passing userIds EVERYWHERE to get them to where they need to be. I believe that schemas really should be able to utilize such things and that means getting them to every invocation of a validation, insert, update, upsert or whatever type call including all those in every used package, even the test ones.

But, maybe that belief is the issue. Perhaps SimpleSchema's readme should have clear warning about never referencing any aspect of the environment outside of the collection or the database in defaultValue or autoValue statements. This practice, in general, might lead to very hard to find problems in normal execution, not just test.

Thoughts?

[tmeasday]

unless you also make sure the right "this" is in place during any SimpleSchema instantiations.

I don't think this is really possible with a simple coding error like:

defaultValue: this.userId

I believe that schemas really should be able to utilize such things

But, maybe that belief is the issue. Perhaps SimpleSchema's readme should have clear warning about never referencing any aspect of the environment outside of the collection or the database in defaultValue or autoValue statements. This practice, in general, might lead to very hard to find problems in normal execution, not just test.

I'm inclined to agree with the second statement. What's the downside of passing the userId directly into the insert call? It doesn't seem like the schema's job to handle the auth side of things. This is why we have methods.

[rhettlivingston]

I'm inclined to agree with the second statement. What's the downside of passing the userId directly into the insert call? It doesn't seem like the schema's job to handle the auth side of things. This is why we have methods.

In this world, I now understand your point and agree. I'm used to a world where these things were implemented in carefully protected stored procedures that few could touch. If the DB wanted to store the userId that changed something, it couldn't be fooled. Every time a record was changed in any way, it was copied to a history table first and the userId and modification time were inserted by the DB. In this world, the DB integrity is being "enforced" by an add-on at a higher level (SimpleSchema) and even that is missing a lot of business rules I normally enforced at the DB level that are now implemented at the even higher level represented by Methods. I suppose my head is swimming with implications of freedom.

If there is a downside to the Method approach, it is that it seems that I'm having to remember to do some DB things in more than one place that I'm used to doing once with a trigger.

I did write an issue to SimpleSchema advising the addition of a note to its README about this difference between defaultValue and autoValue. It would fit right in with other notes they already have and would have likely saved me some of my pain.

[jsep]

I've just release a new version of pm mocha that should report the errors. You guys can try to add practicalmeteor:mocha@2.4.6-rc.2

[rhettlivingston]

@jsep, I just wanted to respond here also that your fix in response to practicalmeteor/meteor-mocha#34 works great! Errors are now making it out all the way to the test console. I don't even have to look at the terminal running the server side. Thank you for your quick response.

I'm going to close this now since that was the biggest problem. Without that, the pain of the other issues would have been far less. And besides, the following comment from @tmeasday at the start really sums it up

I think the headline here is that this stuff is all pretty new and there's a fair bit of room for improvement. Let's make it happen!

Providing detailed writeups on the issues when I encounter them so that they can be fixed quicker is a more constructive approach than questioning the readiness. My bad.