Unit testing NodeJS (without exposing internals)


  • FoxDev

    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.


  • Discourse touched me in a no-no place

    exposing internals

    :giggity:


  • I survived the hour long Uno hand

    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.


  • FoxDev

    @accalia said:

    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 πŸ˜†


  • FoxDev

    @Yamikuronue said:

    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?


  • I survived the hour long Uno hand

    @accalia said:

    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 :)


  • FoxDev

    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?


  • I survived the hour long Uno hand

    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 like Yami 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 the examine method and return output via IRCPrint.

    For Node, why not use EventEmitter? You can emit begin, end, join, part, and message events and let the modules take care of themselves.


  • FoxDev

    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.


  • FoxDev

    @Yamikuronue said:

    For Node, why not use EventEmitter?

    hmm... that seems suspiciously familliar... πŸ˜›

    @accalia said:

    ```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) => {};

  • I survived the hour long Uno hand

    ah, so it is. I went to lunch and came back in the middle of drafting that :)


  • FoxDev

    so i take it then my planned API gets a yami seal of approval*?

    * at least a provisional one


  • I survived the hour long Uno hand

    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? Β―\_(ツ)_/Β―


  • FoxDev

    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 parameters

    all callback functions provided by SockIRC will be function(err)

    πŸ˜›

    doesn't fix it for other projects but at least it's consistent.


  • I survived the hour long Uno hand

    @accalia said:

    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 about who 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.


  • FoxDev

    @Yamikuronue said:

    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.

    @Yamikuronue said:

    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.

    @Yamikuronue said:

    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

    @Yamikuronue said:

    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


  • I survived the hour long Uno hand

    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.


  • FoxDev

    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 into message
    quit: fold into message
    kick: fold into message
    kill: fold into message
    message: Message in a channel
    pm: Private message to the bot
    selfMessage: Message sent by the bot (for logging purposes)
    notice: fold into message or pm depending on destination
    nick: fold into message 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 into message or pm depending on destination

    leaving us with: ['raw', 'message', 'pm', 'selfMessage', 'error']


  • I survived the hour long Uno hand

    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 and where 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.


  • Discourse touched me in a no-no place

    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.


  • FoxDev

    @loopback0 said:

    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'

    @Yamikuronue said:

    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.

    @Yamikuronue said:

    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...


  • I survived the hour long Uno hand

    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.


  • FoxDev

    @Yamikuronue said:

    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.

    @Yamikuronue said:

    You can use mock out the Logger class for testing.

    hmm. good point. and yes that's a thing that would happen ;-)

    @Yamikuronue said:

    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)

    @Yamikuronue said:

    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.


  • Discourse touched me in a no-no place

    @accalia said:

    there's a length restriction?

    512 characters IIRC?


  • I survived the hour long Uno hand

    @accalia said:

    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.

    @accalia said:

    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 :)


  • FoxDev

    @loopback0 said:

    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. ;-)


  • FoxDev

    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 */ 
    }
    

  • FoxDev

    Depends on where you're putting it and what you're using it for?


  • FoxDev

    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....


  • FoxDev

    Can you not do something like you do for the SockSite listener port? Where you set SOCKDEV on the command line or whatever?


  • FoxDev

    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.


  • FoxDev

    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 uses jasmine, but it should be adaptable for grunt. The Module Loader bit appears to be the key to it.


  • FoxDev

    .... any time someone reaches for the vm module as a solution i cringe.....

    that's almost as bad as advocating the use of eval


  • FoxDev

    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 :wtf: solution is using a process.env variable like SOCKDEV; call it SOCKTEST maybe?

    Unless our QA guru @Yamikuronue has a better idea?


  • I survived the hour long Uno hand

    @RaceProUK said:

    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 :)


  • I survived the hour long Uno hand

    @RaceProUK said:

    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" >.>


  • FoxDev

    /me raises hand timidly

    actually at this point i'm using npm test to run my test suite...


  • I survived the hour long Uno hand

    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.


  • FoxDev

    @Yamikuronue said:

    TDEMSYR

    😱

    Nah, it's cool; you're just exposing how little I know about Node, s'all ;)

    @Yamikuronue said:

    @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 πŸ˜›


  • FoxDev

    @Yamikuronue said:

    "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...


  • I survived the hour long Uno hand

    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


  • FoxDev

    @Yamikuronue said:

    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


  • I survived the hour long Uno hand

    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?


  • I survived the hour long Uno hand

    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 and begin as well as the name, descrip, et cetera

    SockDice 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.


  • FoxDev

    @Yamikuronue said:

    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)


  • I survived the hour long Uno hand

    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.


  • FoxDev

    @Yamikuronue said:

    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.


  • FoxDev

    @Yamikuronue said:

    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!)


  • I survived the hour long Uno hand

    Good plan. I'm going to sleep now :)


  • :belt_onion:

    @Yamikuronue said:

    [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 on TRACE. As a bonus, the majority of the different logging libraries for Java can all talk each other's protocols so even if your third.party.json dependency is using slf4j and one of its dependencies is using java.*.logging you can still manage everything through log4j.

    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.


Log in to reply