Tests for the Session API#3967
Conversation
There was a problem hiding this comment.
Please use 4 spaces instead of tabs.
There was a problem hiding this comment.
Darn, that was unintentional. I hate tabs. Can we modify the tslint to pick up on this, too?
There was a problem hiding this comment.
Yup, that's a good idea.
|
looks good to me, along with @DanielRosenwasser's comments. @zhengbli can you take a look as well. |
|
Re: All my responses mentioning tslint - on further investigation most of them should have been picked up by out current tslint settings according to the tslint documentation, but these ones didn't seem to get reported when I ran a pass. I looked into it a bit and found the following: This is probably a result of having a |
There was a problem hiding this comment.
Could you define a type alias for Session & {enqueue: (msg: protocol.Request) => void}?
You can also do this within functions so it can be local to the containing function.
There was a problem hiding this comment.
TS doesn't allow local type aliases (? has this changed recently?) and I'd been trying to keep all the types internal to the test. So I'd have to hoist the alias all the way out to the ambient level despite no other tests or types in the module depending on it. Eh?
Makes me want local type aliases.
|
@weswigham thanks for pointing out the lack of TSLint rules, I've created #3994 to track the issue. |
There was a problem hiding this comment.
Should this be a method call?
There was a problem hiding this comment.
Honestly the line probably shouldn't be there at all... Really, I effectively wanted an assert(true) after calling session.exit() to indicate that the process should still be running. I'm not really sure how to test that a function which should be a noop is, in fact, still a noop. (Beyond literally stringifying the function and checking it's source, which seems extremely tautological)
Actually, in every case Session gets subclassed - maybe we should just mark Session as abstract and exit() as an abstract method (then remove the test)? I think that may be more in line with what is supposed to occur.
There was a problem hiding this comment.
I'd be fine with that as a separate PR. In the mean time, consider removing the test.
Additionally, I added visibility modifiers to all the members, so we declare public as little as is required.
When merged, this should close issue #3813.