Critique my newbie approach to testing

I've just begun a small hobby project building a telegram bot. I'm using the opportunity to start off building AND testing. Typically I shy away from tests so it's time to fix that issue and get on with it.

That being said I'm struggling to understand a few points and I have included an example of a module I'm testing as well as the test cases. I've decided to go with Jest as I have this quite nicely integrated with VSCode.

So first off general questions:

1. My testing doesn't seem to follow too much of a structured approach when it comes to WHAT I should test for. I aim to check for the happy path and a couple of bad case scenarios. Does anyone have a general approach to what they should be testing for? e.g. bad inputs, correct inputs, expected outputs?

2. What depth of test do you go for? Do you simply test the overall function works or do you try and isolate particular elements of a function? I've read about "black box" testing and not caring about implementation but how detailed/low level should you be shooting for? I could quite easily just test the public interfaces and job done?

3. If I have some internal functions which aren't exported I may want to specifically test their functionality too. Is this a no no?

4. What are the best conventions for the "expect" descriptions? In my examples I've just added a sentence which makes perfect sense to me but probably not to another person. Should I be going for the "It should return xxx based on xxx" or some other nice framework description?

Here's the module:

const dbConnection = require('../Services/db/Db')
const config = require('../Config')
const USER_COLLECTION = config.envs[process.env.NODE_ENV].databaseConnection.DB_USER_COLLECTION

// Set database connection to use the User table
const userTable = dbConnection.collection(USER_COLLECTION)

class User {
    constructor(pNick = '', firstName = '', userName = '', phoneNumber = '', userIdentifier = 0) {
        this._pNick = pNick
        this._firstName = firstName
        this._userName = userName
        this._phoneNumber = phoneNumber
        this._userID = userIdentifier
        this._isAdmin = this.isAdmin()
    }

    static getByID(userID = 0) {
        return new Promise((resolve, reject) => {
            userTable.findOne({
                _userID: userID
            }, function (err, result) {

                // If error reject
                if (err) {
                    return reject(err)
                }

                // Return user records if found
                return resolve(result)
            })
        })
    }

    // Search for a user by their pNick, includes partial matching
    static getBypNick(pNick = '') {
        // Define our search criteria regex and normalise to lower case
        const userSearchRegex = new RegExp(`^${pNick.toLowerCase()}`, 'i')

        return new Promise((resolve, reject) => {

            // Search user collection for pNick using regex like search and return array of results
            userTable.find({
                _pNick: userSearchRegex
            }).sort({
                _pNick: 1
            }).toArray(function (err, result) {

                // If error reject
                if (err) {
                    return reject(err)
                }

                // Return user records if found
                return resolve(result)
            })
        })
    }

    isAdmin() {
        if (this._userID === config.adminUser.ADMIN_ID) {
            return true
        }
        return false
    }

    isAuthenticated() {
        if (this._pNick.length > 1) {
            return true
        }
        return false
    }

}

module.exports = User

Here's my tests:

const User = require('./User')
const user = new User()

test('Returns a new user with sensible default values', () => {
    expect(user).toBeTruthy();
    expect(user).toEqual({
        _firstName: "",
        _isAdmin: false,
        _pNick: "",
        _phoneNumber: "",
       _userID: 0,
       _userName:""
    })
})



test('Returns a new known user', () => {
    const admin = new User('munkee','Chris','munkeeusername','000',234234)
     const expectation =  {
        _pNick: 'munkee',
        _firstName: 'Chris',
        _userName: 'munkeeusername',
        _phoneNumber: '000',
        _userID: 234234,
        _isAdmin: false

     }
    expect(admin).toEqual(expectation)
    expect(admin._isAdmin).toBeFalsy()
})

test('Returns a new unknown user', () => {
    const admin = new User('','Chris','munkeeusername','',234234)
     const expectation =  {
        _pNick: '',
        _firstName: 'Chris',
        _userName: 'munkeeusername',
        _phoneNumber: '',
        _userID: 234234,
       _isAdmin: false

     }
    expect(admin).toEqual(expectation)
    expect(admin._isAdmin).toBeFalsy()
})

test('Returns a new admin user', () => {
    const admin = new User('munkee','Chris','munkeeusername','000',60178217)
     const expectation =  {
        _pNick: 'munkee',
        _firstName: 'Chris',
        _userName: 'munkeeusername',
        _phoneNumber: '000',
        _userID: 60178217,
       _isAdmin: true

     }
    expect(admin).toEqual(expectation)
    expect(admin._isAdmin).toBeTruthy()
})

test('Identify an authenticated user', () => {
    const authableUser = new User('munkee','Chris','munkeeusername','000',60178217)
    const expectation =  {
       _pNick: 'munkee',
       _firstName: 'Chris',
       _userName: 'munkeeusername',
       _phoneNumber: '000',
       _userID: 60178217,
      _isAdmin: true

    }
   expect(authableUser).toEqual(expectation)
   expect(authableUser.isAuthenticated()).toBeTruthy()
})

test('Identify a non authed user', () => {
    const nonAuthUser = new User('','Chris','munkeeusername','',234234)
     const expectation =  {
        _pNick: '',
        _firstName: 'Chris',
        _userName: 'munkeeusername',
        _phoneNumber: '',
        _userID: 234234,
       _isAdmin: false

     }
    expect(nonAuthUser).toEqual(expectation)
    expect(nonAuthUser.isAuthenticated()).toBeFalsy()
})

test('Returns no results when searching for a user with unknown ID', () => {
    expect.assertions(1)
    return User.getByID(2048098).then((result) => {
        expect(result).toBeFalsy()
    })
});

test('Returns no results when searching for an invalid ID type e.g. string', () => {
    expect.assertions(1)
    return User.getByID('string').then((result) => {
        expect(result).toBeFalsy()
    })
});

test('Returns user munkee results when searching with exact ID', () => {
    expect.assertions(2)
    return User.getByID(60178217).then((result) => {
        expect(result).toBeTruthy()
        expect(result._pNick).toEqual('munks')
    })
});

test('Returns the user munkee when searching via pnick', () => {
    expect.assertions(2)
    return User.getBypNick('munks').then((result) => {
        expect(result).toHaveLength(1)
        expect(result[0]._pNick).toEqual('munks')
    })
});

test('Finds the user munkee from partial pnick search', () => {
    expect.assertions(2)
    return User.getBypNick('mun').then((result) => {
        expect(result).toHaveLength(1)
        expect(result[0]._pNick).toEqual('munks');
    })
})

test('Returns an empty array when not finding a user', () => {
    expect.assertions(1)
    return User.getBypNick('xxxx').then((result) => {
        expect(result).toHaveLength(0);
    })
});

Brutal honesty please :)

Comments

[Leighton Darkins]

This is a really interesting post, and good on you for seeking feedback!

I'll be very interested to see the responses you get here. Approaches to and opinions on, unit testing are generally wildy variant.

1: This is an example of where laying out expectations for your code, before you write it can be super helpful. I'm not going to start preaching about TDD, but even if you just jot down on a piece of paper what each function should do before you write it, you'll have a great starting point for your test assertions.

Take getBypNick() for example. As I consider that function I'd think to myself:

• What does it do when:
  • it doesn't receive an argument?
  • it receives a valid argument?
    • that doesn't match a user?
    • that does match a user?
  • What does it do when it receives an invalid argument?
    • What does it do when it receives a number / object / boolean as an argument?

This could go on for a while... but as a result, I walk away with a good set of questions, and expectations that can de translated directly into tests.

Using this approach, I can cover an assumption you've made in your implementation: That you can always call .toLowerCase() on pNick. Right now that would just throw a syntax or type error for number / object / boolean. You can decide for yourself what this function should do in those cases.

2: In general, I just test the public interface of the function. If a function is doing too much, or there are too many assertions covering edge cases in it's internals, I'll extract those internals into separate functions and test them in isolation.

3: You'll run into differing opinions here. Some will say private methods are implementation details that unit tests shouldn't have access to or care about. That would break encapsulation, y'see? Others will say that leaving private methods uncovered is a recipe for disaster. Gotta have that 100% code coverage :P.

Luckily (kind of) in JS "private" functions aren't really a thing (for the time being), so they're always exposed. And given that fact, I generally test "private" functions on JS classes.

It's not breaking encapsulation if it was never encapsulated in the first place ;).

4: This is another source of vastly differing opinions.

I don't like the word "should". To me it implies some amount of variance in the expectation. I like a more binary approach, it either does something, or it doesn't do something.

Most of my it or test blocks look like this:

 it('returns undefined when given a number as an argument, () => {
    expect(thingUnderTest(undefined)).toBeUndefined()
 })

[ImTheDeveloper]

Excellent response thanks! I've amended a bunch of my test cases now to match your suggestions and it's slowly starting to make more sense to me. As you say there doesn't seem to be much of an approach agreed out there and I've also been reading a number of other posts on dev.to with a similar sentiment, including the misinterpretation of TDD being purely unit testing, when in-fact its an approach to unit testing.