5
\$\begingroup\$

I'm new to unit testing so maybe I have this all wrong, but it seems kind of useless. I want to reduce bugs and integrate some tests with a build process however I'm not seeing the benefit at the moment. How can I improve the quality of tests, like the one below?

/* script */
SetAttributes = function(el, attrs) {
  /**
   * @method simple for in loop to help with creating elements programmatically
   * @param {object} el - HTMLElement attributes are getting added to
   * @param {object} attrs - object literal with key/values for desired attributes
   * @example SetAttributes(info,{
   *    'id' : 'utswFormInfo'
   *    'class' : 'my-class-name'
   * });
   */

  for (let key in attrs) {
    if (attrs.hasOwnProperty(key)) {
      el.setAttribute(key, attrs[key]);
    }
  }

  return el;
};

/** \/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/ **/


/* unit test */
describe("SetAttributes is for setting multiple attributes at a single time.", function() {

  /** SetAttributes */
  it("should not be null because it returns an HTMLElement with newly added attributes.", function() {
    expect(SetAttributes(document.createElement('div'), {
      defer: true,
      src: null,
      type: 'text/javascript'
    })).not.toBe(null);
  });
  it("should not be a number because it returns an HTMLElement with newly added attributes.", function() {
    expect(SetAttributes(document.createElement('div'), {
      defer: true,
      src: null,
      type: 'text/javascript'
    })).not.toBe(0);
  });
  it("should not be a string because it returns an HTMLElement with newly added attributes.", function() {
    expect(SetAttributes(document.createElement('div'), {
      defer: true,
      src: null,
      type: 'text/javascript'
    })).not.toBe('div');
  });
  it("should not be undefined because it returns an HTMLElement with newly added attributes.", function() {
    expect(SetAttributes(document.createElement('div'), {
      defer: true,
      src: null,
      type: 'text/javascript'
    })).not.toBe(undefined);
  });
  it("should not be a false value because it returns an HTMLElement with newly added attributes.", function() {
    expect(SetAttributes(document.createElement('div'), {
      defer: true,
      src: null,
      type: 'text/javascript'
    })).not.toBeFalsy();
  });
});

\$\endgroup\$
5
\$\begingroup\$

First, your function should follow JS conventions and be camelCase, not PascalCase. The latter is for constructors, but this is just a function. So for the following I'll call it setAttributes instead.

As for your tests/spec: You should structure - and name - them differently. Treat the descriptions as documentation you can read through.

E.g. the describe call should just be describe("setAttributes()"). It's a headline, not a paragraph. The following tests are what constitute the actual description. So, your describe call should not itself do anything but say what's being described. That could be a concept (e.g. describe("user login flow")) but in this case, it's a named function, so let's "describe setAttributes()":

Your function has one primary purpose: It sets multiple attributes. However, you aren't actually testing that! I'd say the most important test would be:

it("sets an element's attributes from an object of names and values", function () {
  var element = document.createElement('div');
  setAttributes(element, {
    style: 'color: red',
    id: 'testElement'
  });
  expect(element.getAttribute('style')).toEqual('color: red');
  expect(element.getAttribute('id')).toEqual('testElement');
});

(Sidenote: There are two expectations in this test case, but you should usually aim for just one expectation for each test case, if you can. This forces you to be precise in your tests, and not test too much at once. Same rule as for writing functions: Do one thing, and do it well. Here, having two expectations isn't too bad, since they're not really testing different things, but in most other cases, aim for just 1.)

Secondly, setAttributes() should return the element it was given:

it("returns the element", function () {
  var element = document.createElement('div');
  expect(setAttributes(element, {})).toEqual(element);
});

And done. That's your function's specification.

For now, anyway. If you have bugs later, start by writing a test case triggers the bug, but also describes how it should work (and therefore fails when you run it), and then fix you code until the tests start passing again. That way, you test suite grows naturally, based on actual bugs encountered.

What you're doing right now is testing the negative again and again, which, yes, is pointless. Your tests basically say that the return value is not specifically zero, it's not a specific string, it's not a given boolean, etc.. You can do that forever.

In short: There's an infinite number of things it shouldn't be, and you can't check them all.

So what you want to check is the positive instead: Does it do what it should do. I.e. do I get the element back that I passed in? If yes, then you can be pretty confident that you're not going to get numeric zero all of a sudden. Especially since you keep calling your function with the same arguments; unless JavaScript is suddenly completely broken, you will get the same result. And it'll never be zero, a string, a boolean, or... etc..

You basically only want to test negatives when you don't know the exact value you're expect, but know that's there's one thing it certainly shouldn't be. Again, consider how your spec is worded; in some cases you want it to document what definitely won't happen, and then cover what will happen in another test.

So if you for instance have a function that is made to be resilient against bad input, you could make spec that says that it won't throw a TypeError on bad input, and one that says it will return null (or something).

So in all:

describe('setAttributes()', function () {
  it("sets an element's attributes from an object of names and values", function () {
    var element = document.createElement('div');
    setAttributes(element, {
      style: 'color: red',
      id: 'testElement'
    });
    expect(element.getAttribute('style')).toEqual('color: red');
    expect(element.getAttribute('id')).toEqual('testElement');
  });

  it("returns the element", function () {
    var element = document.createElement('div');
    expect(setAttributes(element, {})).toEqual(element);
  });
});

Try reading the spec out loud:

setAttributes() sets an element's attributes from an object of names and values [and] returns the element

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.