Unit testing NodeJS (without exposing internals)
-
i think this is mostly for @Yamikuronue but i welcome input from anyone who codes JS and does unit testing/TDD.
so i should have been unit testying my js code for sockbot and socksite for ages now. and i'm finally getting
so i start working with mocha+chai and i run into a problem. I want to unit test my helper functions that are internal to my modules, but i don't want to expose them as exports because they are internal for a reason.... so what do i do? do i conditionally expose them when testing? do i write the tests straight in the module?
constructive advice requested and appreciated.
-
exposing internals
-
You test against your API. A single function should do a single "thing". If it calls upon helper functions for DRYing out the code, that's fine, but that's part of the "thing" it does, not a separate thing to test.
That said, when I was writing my card module, I did expose the "business logic" functions as exports for testing. Each of them represents one logical action the bot can take; just because they're not called directly doesn't mean they shouldn't be part of the API for that module, so to speak. If I were to move the Discourse-handling logic up a level (as I probably would in a more OO-savvy environment), I would expose those functions as exports and it'd work easily as a plugin for, say, an IRC bot as well ;) After all, why should my module know anything about Discourse, notification types, trust levels, etc?
In short: whenever you have testing issues, reconsider your architecture. You've probably muddled your separation of concerns.
-
do i write the tests straight in the module?
I'm not that familiar with TDD, but to me, that is a huge ; it's my understanding that tests are kept separate from the code they're testing.
@accalia said:do i conditionally expose them when testing?
Unless someone has any better ideas, this seems to be the best way to do it.
β¦and as I was typing this, the SockDrawer testing guru has posted
-
In short: whenever you have testing issues, reconsider your architecture.
yeah the architecture in Sockbot is a fxxing mess. i need to sort that out eventually, but for now it works.... kinda.
SockSite is better
for SockIRC my architecture is pretty damn simple for the modules.
// a minimal IRC module for SockIRC /** * Default configurations. will be merged with config file when passed into begin */ exports.defaults = {} /** * config: configuration for the module. read from a config file and passed in * client: IRC(/discourse?) client abstraction. has methods like 'say()' and 'action()' to do bot things (will be mocked in test) * events: a nodejs EventEmitter that will emit a known set of events based on IRC/Bot activity */ exports.begin = (config, client, events) => {}; /** * called when the bot is shutting down to allow for long running processes/logfiles to be terminated cleanly */ exports.shutdown = (callback) => callback();
Any suggestions on changing the architecture then? or on approaches to testing this architecture?
-
Any suggestions on changing the architecture then?
Here's what I'd suggest:
In the short term, to get your unit tests up and running: put all the Discourse-based logic in one method and the actual functionality in another. Add the functionality method to your module.exports and unit test that.
Step two: separate the two functions into two separate modules. Have the Discourse-adapter module require the actual functionality module. Unit test the discourse-adapter module using Sinon to mock out the functionality.
Step three: make each of them their own package in their own repo. This is basically the way Grunt does things: grunt-foo will require foo and provide the interface grunt is expecting to make foo work. That also makes plugin development by other people easier :)
-
that makes sense for sockbot and was about what i was planning, but that's a ways off right now. i want to get SockIRC and SockSite stable and feature complete first. (actually i'm thinking of apeing the SockIRC architecture for SockSite. but that's a huge rewrite)
Any suggestion on the architecture i'm thinking of for SockIRC?
-
My own IRC bot's plugin interface consists of one method*. It basically makes all the modules Observers of the "message received" event that the main bot can emit. Without good libraries, I've had to do it manually (in perl, so it's a heaping WTF), but it basically consists of the signature
examine(input, where, who)
. Where and who might be identical, for a privmsg, but they basically let the module know where it's responding and what nick they might want to address during parse (for things likeYami rolled: 2d6
).The core bot exposes a public method
IRCPrint(where,message)
that will take care of the actual sending of messages (both to chans and nicks); this came in real handy when I wanted some longer output as I could put cutscript functionality in there. Everything specific to IRC is in the main Logios namespace; modules handle their own input parsing in theexamine
method and return output viaIRCPrint
.For Node, why not use
EventEmitter
? You can emit begin, end, join, part, and message events and let the modules take care of themselves.
-
That sounds a bit like how I was envisaging NetBot (the software, not the account) would work; the core would expose an interface with basic operations and events, and the modules would expose an interface with one method that accepts an instance of the core interface. The modules would then just subscribe to events and call the operations methods.
-
For Node, why not use EventEmitter?
hmm... that seems suspiciously familliar...
```javascript
/**- config: configuration for the module. read from a config file and passed in
- client: IRC(/discourse?) client abstraction. has methods like 'say()' and 'action()' to do bot things (will be mocked in test)
- events: a nodejs EventEmitter that will emit a known set of events based on IRC/Bot activity
*/
exports.begin = (config, client, events) => {};
-
ah, so it is. I went to lunch and came back in the middle of drafting that :)
-
so i take it then my planned API gets a yami seal of approval*?
* at least a provisional one
-
Well, for that piece of it, I guess
Annoying thing about Node culture: they're like, allergic to documenting parameters. What params will the event have when it's emitted? What params will my callback receive? Β―\_(γ)_/Β―
-
this at least i'll fix (because node-irc is discoursistent about it)!
all events will emit these parameters:
/** * nick {string|null} nick of the user that created the action or null if server initiated * target {string|null} target (channel/user) of the action or null if global action * text {string} text of the message or reason for the action * message {object} raw (but parsed) IRC message if provided by underlying service, empty object otherwise */
with the documented exception that the
raw
event will always provide null for the first three parametersall callback functions provided by SockIRC will be
function(err)
doesn't fix it for other projects but at least it's consistent.
-
the raw event
What's it for?
In years of running my own IRC bot I've never needed anything other than nick, target, text. And I've never needed to parse a global action, either. YAGNI, KISS.
I've also moved toward using question-words (who,where, and what) to describe params instead of nouns (nick and target). That helps in weird edge cases sometimes -- you can always post back to
where
you got the event in the first place and talk aboutwho
did the event, even if they're one and the same. But that's semantics.Consider emitting two events on each message: one general "message received", and one "Privmsg received" (or whatnot). This lets bots that don't care about where the message came from subscribe to the general case and get all messages through the same conduit (just about all Logios modules fall into this case), while modules that only respond on PM or action or channel message can subscribe to just the type they want.
-
What's it for?
includes extra information about the event. i get it for free in most of the events emitted by node-irc so i'm keeping it. both because it's minimal extra effort and because i plan on shoving the post JSON in that parameter when i reuse the plugin API for discourse. anything that doesn't care about the raw will just define the first three parameters and be blissfully unaware of the extra one.
And I've never needed to parse a global action, either.
again, i'm adding that because i don't want to filter them out. i get them emitted by node-irc anyway and a third party module might care about it. i expect most of the time no one will be listening go those events so nothing will happen.
I've also moved toward using question-words (who,where, and what) to describe params instead of nouns (nick and target)
ooh. good idea./me pads off to change the names before it's too late
Consider emitting two events on each message: one general "message received", and one "Privmsg received" (or whatnot).
yep got that.
current events i emit:
motd: MOTD message from server
topic: Channel topic set to something new
join: X has joined the channel
part: X has left the channel
quit: X has quit
kick: X was kicked
kill: X was killed
message: Message in a channel
pm: Private message to the bot
selfMessage: Message sent by the bot (for logging purposes)
notice: someone/notice
'd the bot
nick: X changed their nick to Y (for logging)
raw: i got this IRC packet.... wanna look at it (probably also emits one of the other messages too)
error: i got this IRC error packet.... watcha want to do about it?
action: someone said '/me something'which is basically all except the CTCP events that node-irc emits. because it was easy to hook them up and they're all easily testable so if soneone wants them thy're there and if not then nothing happens
-
Everything you write you have to test. And support, if this catches on and becomes a thing; you know the saying about writing like the guy who maintains the code is a deranged axe murderer? I like to plan architecture* like this is the next Node.JS and I'll be stuck supporting it for 20+ years and millions of users.
I'd only write the most common use cases and add later. But that's me.
-
hmm... fair enough.
so leave
raw
in so if you're interested in any event i don't explicitly break out for you it's available for you to deal with... then strip it down to....join: fold into
message
part: fold intomessage
quit: fold intomessage
kick: fold intomessage
kill: fold intomessage
message: Message in a channel
pm: Private message to the bot
selfMessage: Message sent by the bot (for logging purposes)
notice: fold intomessage
or pm depending on destination
nick: fold intomessage
or pm depending on destination
raw: i got this IRC packet.... wanna look at it (probably also emits one of the other messages too)
error: i got this IRC error packet.... watcha want to do about it?
action: fold intomessage
or pm depending on destinationleaving us with:
['raw', 'message', 'pm', 'selfMessage', 'error']
-
If you really think you'll handle all those, I'd roll up join, part, quit, kick, nick, and kill into some sort of UserAction event (actions performed on the user list), and roll up message, pm, notice, and action into MessageReceived. You can tell a message is a privmsg because
who
andwhere
are identical, and in practice, nobody cares if you do "/me !roll 2d10" instead of just messaging the chan. Bots that need to keep track of the users in the chan will subscribe to all of the UserAction events anyway -- they have to, to maintain their lists accurately.Will modules need to act on self messages? Mine all just have access to a logging function in the core module they can use to log their own events.
-
It's been a lot of years since, but my first non-VBA "programming" was writing scripts for mIRC - the raw was pretty handy for those.
And I know we're not talking about that here, but the point is that the raw IRC stuff is handy. Might as well be there and not used.
-
And I know we're not talking about that here, but the point is that the raw IRC stuff is handy. Might as well be there and not used.
that's the point of the fourth parameter. the object will look like this:
message = { prefix: "The prefix for the message (optional)", nick: "The nickname portion of the prefix (optional)", user: "The username portion of the prefix (optional)", host: "The hostname portion of the prefix (optional)", server: "The servername (if the prefix was a servername)", rawCommand: "The command exactly as sent from the server", command: "Human readable version of the command", commandType: "normal, error, or reply", args: ['arguments', 'to', 'the', 'command'], }
from: http://node-irc.readthedocs.org/en/latest/API.html#'raw'
If you really think you'll handle all those, I'd roll up join, part, quit, kick, nick, and kill into some sort of UserAction event
good point. will do.Will modules need to act on self messages? Mine all just have access to a logging function in the core module they can use to log their own events.
hmm... a good question i'm not sure. will have to think on it. given that i was planning on implementing logging as a core module it would be handy, but i could roll it directly into core without making it a module...
-
If I were writing it over, I might want a standalone logger like log4j that I've been using in my Java projects; each class just calls Logger.getInstanceForClass(this.class) (which, in practice, seems to just put the class that logged the message into the log statement when it logs, so, sure, whatever) and bam, logging. You can use mock out the Logger class for testing.
The core module should abstract away all that IRC crap: different message types that mean the same thing, NickServ interaction, connecting and reconnecting, et cetera. Give your modules a nice interface to work with that hands them only as much as they actually need. Technically logging's not part of that, but that's where I put it. I also, as mentioned above, made my own wrapper for sending messages to IRC to handle message length restrictions.
-
The core module should abstract away all that IRC crap: different message types that mean the same thing, NickServ interaction, connecting and reconnecting, et cetera.
won't even be exposed to modules that. that'll be set by the configuration file and modules can't change it.
You can use mock out the Logger class for testing.
hmm. good point. and yes that's a thing that would happen ;-)
Technically logging's not part of that, but that's where I put it. I also, as mentioned above, made my own wrapper for sending messages to IRC to handle message length restrictions.
makes sense. i'll probably also rate limit messages to prevent a bad module flooding a channel with messages (something like a max of ten messages a second. still a lot but not so much as to take down the channel or server if the bot goes bonkers)
I also, as mentioned above, made my own wrapper for sending messages to IRC to handle message length restrictions.
there's a length restriction? huh. i guess i don't tend to talk that much in one message.
-
-
rate limit messages
oh right, I forgot about that. My IRC framework turned out to do that for me, but that's also crucial to good bot behavior.
there's a length restriction?
Grab a client without a built-in cutscript (mIRC has one now) and paste paragraphs into it. RPers have long used elaborate fancy cutscripts (or the more simple <c> notation) to get around the length limit when they've got the muse :)
-
512 characters IIRC?
yeah... i've never got anywhere neat that long in my own typing.
though that does explain why yami soemtimes posts a chunk of text as multiple messages. ;-)
-
okay then......
on a scale from 0 to "hand in your programmer's license"..... how bad would it be to do
if (typeof describe === 'function'){ /* register a bunch of exports to test */ }
-
Depends on where you're putting it and what you're using it for?
-
into my js files to contitionally export helper functions i really want to keep internal (not exported) for testing
it's a hack, but it works and i'm not finding much in the way of reflection to help me get at the things internals any other way.
in C# i'd just declare the test class as being "friends" with the target class and get access to the internals that way....
-
Can you not do something like you do for the SockSite listener port? Where you set
SOCKDEV
on the command line or whatever?
-
also an option...
i'm really looking for the least bad way to do this.
like i have functions that manipulate the internal state of an object. i'd really really really like to test that robustly and in isolation from everything else while also making it so the function is not accessible to someone calling
require('mymodule')
on it.
-
The issue really is JavaScript has no built-in visibility control w.r.t. methods and variables; Node fakes it using
exports
, but of course it's an imperfect solution.We use
grunt
for unit tests, yes? This usesjasmine
, but it should be adaptable forgrunt
. TheModule Loader
bit appears to be the key to it.
-
.... any time someone reaches for the
vm
module as a solution i cringe.....that's almost as bad as advocating the use of
eval
-
I'll to your greater experience on that one ;)
All my Googling comes up with either something involving
vm
, or using some sort of conditional to turn on extra exports. The only one that didn't do either of those requires the use of a build and deployment server; since we deploy direct from repo, we can't really use that solution.
IMHHO, the least solution is using aprocess.env
variable likeSOCKDEV
; call itSOCKTEST
maybe?Unless our QA guru @Yamikuronue has a better idea?
-
We use grunt for unit tests, yes? This uses jasmine, but it should be adaptable for grunt.
TDEMSYR.
Jasmine:Mocha::Grunt:Gulp
Grunt is a task runner. Mocha is the framework we're using for unit testing :)
-
Unless our QA guru @Yamikuronue has a better idea?
Put up with exporting more than you wanted until you get a chance to do a proper refactor. What's the worst that could happen? "Oh no, someone using my module gets extra functionality" >.>
-
/me raises hand timidly
actually at this point i'm using
npm test
to run my test suite...
-
Awesome. @RaceproUK, start using Makefiles so then we cover all three competing standards ;)
How we run them is honestly not a big deal to me. It's open-source, do what you're comfortable with. As long as we're writing compatible tests so we can all execute all the tests I see no problem with it. I use Grunt at work because it's better for CI since you can define other build-time tasks and run them in sequence.
-
TDEMSYR
Nah, it's cool; you're just exposing how little I know about Node, s'all ;)
@RaceproUK, start using Makefiles so then we cover all three competing standards
Can I use MSBuild files instead? Since I do all my dev on Windows
-
"Oh no, someone using my module gets extra functionality"
... that honestly feels ickier to me than conditionally exporting things. at least with a conditional export you have to put a little effort into getting access to the things i don't want you to have access too.
but then i'm not a QA person by training...
-
But why don't you want hypothetical people being able to call your business logic functions tho? That's what's got me all confused. Especially since if you follow my suggestions above for refactoring they'll be exposed eventually anyway
-
Awesome. @RaceproUK, start using Makefiles so then we cover all three competing standards
well my plan for when i implement grunt in this project is just have the test task for grunt call
npm test
that way you test with whichever one you want (travis can have grunt) and they're always the same
the test command right now is:
./node_modules/eslint/bin/eslint.js . && ./node_modules/mocha/bin/mocha -st --compilers js:mocha-traceur
-
Grunt has its own plugins already for eslint and mocha though. I'm no Node expert, but I didn't think you usually put eslint in npm test?
-
To use a concrete example, I've popped open SockDice.
To me, you have two sets of potential exports that ought to be separate modules:
discourse-SockDice exposes
onNotify
and andbegin
as well as the name, descrip, et ceteraSockDice should be exporting
rollDice
,rollAllDice
,parse
,preRollDice
, et cetera, so that discourse-SockDice can use them. Then you can have irc-SockDice re-use the same SockDice module.Instead you're hiding all the business logic and exporting only the Discourse interface, which makes it impossible to re-use the logic (or unit test it)
and I'm not at all sure why you're so intent on that.
-
But why don't you want hypothetical people being able to call your business logic functions tho?
because they're not business logic functions. they're stuff like "you've connected to the server now set up the event handlers and give me back my event emitter." i want to test that i hooked up all the events i expected and that when the mocked server sends the events i expect they're handled correctly.
that's not a piece of functionality i want accessible all on it's own and i want to break it out of the
connect()
function because that bit of code is about 40 lines all on its own and it's functionally separate from the bit that reads the config file, sets up the IRC client, loads the modules, etc.the business functionality is the whole
connect()
function, but i want to test it piecemeal to better isolate the individual steps involved (and so i can inject the appropriate mocks at test time to really dive into everything)
-
ah, so you're talking about a different class of function than I was picturing. See my example above.
Honestly, I wouldn't unit test private functions except by extension when testing the unit that calls them. Not when it's impossible to do something like a friend class to easily get at them. The unit extends from the logical entry point until the logical seams where it calls into other classes/modules or returns. Everything inside the object under test is a black box.
-
Instead you're hiding all the business logic and exporting only the Discourse interface, which makes it impossible to re-use the logic (or unit test it)
that would be an example of the horrible architecture of sockbot. i need to fix that.
and i will fix that.
the part i'm talking about is testing core, which sits above the modules. there's a lot of things in core that the modules shouldn't be able to poke.
-
Honestly, I wouldn't unit test private functions except by extension when testing the unit that calls them. Not when it's impossible to do something like a friend class to easily get at them. The unit extends from the logical entry point until the logical seams where it calls into other classes/modules or returns. Everything inside the object under test is a black box.
when i test the modules that's exactly how i will test and i will make sure my module API supports that.
but for the core.... it's harder to split things up. at least for me.
i'll write up the tests and the core and let you critique it before i start on the modules. catch anything wrong before it has a chance to spread to the modules (rewriting sockbot to improve architecture is going to SUCK!)
-
Good plan. I'm going to sleep now :)
-
[E]ach class just calls
Logger.getInstanceForClass(this.class)
(which, in practice, seems to just put the class that logged the message into the log statement when it logs, so, sure, whatever) and bam, logging.Random side note; almost all Java loggers are actually hierarchical loggers (each logging instance is inserted into a tree of loggers, based on the package of the class). This is why they want the class they are logging for. The hierarchy makes it easy to do things like silence all the output of the
third.party.json.parser
while still keeping your own logging onTRACE
. As a bonus, the majority of the different logging libraries for Java can all talk each other's protocols so even if yourthird.party.json
dependency is usingslf4j
and one of its dependencies is usingjava.*.logging
you can still manage everything throughlog4j
.NodeJS, being newer than Java has several loggers of this type (https://github.com/seanmonstar/intel is a good example), but none of them interoperate - and most libraries don't ship with much useful logging in the first place.