Testing best practices

Testing best practices

Add more best practices here!

Don't use parens in test names

Parens (and possibly other characters) in the name of a test keep it from being grep-ed, and therefore from being run by itself.

• Can't be grep-ed: it("calls function foo()", () => { ... });
• Okay: it("calls function foo", () => { ... });

Parens do not appear to cause problems when used in naming describe()s.

Retrieving a number attribute value from the DOM / D3 Selection

When retrieving the number attribute off a DOM element, usually the preferred method is to first wrap the element into a D3.Selection and then use TestMethods.numAttr(selection, attr) to get at the number. Due to the many ways that we can extract number information from a string, we should be more consistent with how we do this.

Always factor out constants

Constants that are used in tests, such as element sizes or CSS class names or other strings or numbers, should always be factored out into variables. This practice prevents cases where typos cause tests to pass when they shouldn't: In the above example, the mistyped class name causes the select() call to find nothing, meaning the later assert() will always pass.

Making tests more readable

it() descriptions should read like a sentence

it("value()", () => { ... }); // doesn't explain what's being tested
it("can get and set value()", () => { ... }); // good: reads like a sentence, explains what the test does

Assert error throwing should include the error message.

(<any> assert).throws(() => c.foo(), Error, "", "The reason"); // bad
(<any> assert).throws(() => c.foo(), Error, "Error message", "The reason"); // good

The reason for that is that if we are expecting an error, but we are getting a different error, we should catch that as unexpected behavior. The reason against this change was code fragility (if we change the error message we don't want to duplicate the code, but we already do that for any of the behavior change in code that reflects into tests)

Assert messages in loops should include the index

for (let i = 0; i < n; i++) {
  assert.strictEqual(array[i], expected, `object ${i} has expected property`); // template string
}

Doing so makes it easier to debug where in the loop things went wrong. This should also be used in forEach() loops.

Use assert.operator for greater-than or less-than checks

assert.isTrue(a > b); // does not produce a helpful error message
assert.operator(a, ">", b); // better

---

Checking the size of a D3 Selection

The "length" of a D3 Selection is always 1, regardless of how many elements are in it:

d3.select().length; // 1, not 0

The correct way to check the size of the Selection is to assert that its size() is equal to a particular value:

assert.strictEquals(selection.size(), expectedSize, message);

Iterating over a d3 selection

Use

whateverSelection.selectAll("line").each(function() {
  let lineSelection = d3.select(this); // "this" refers to the DOM node inside the loop
});

Instead of hacky stuff like

whateverSelection.selectAll("line")[0].forEach((line) => {
  let lineSelection = d3.select(line);
});

Always check the size of a d3 selection

let edges = dbl.content().selectAll("line"); // this selection
assert.strictEqual(edges.size(), 4, "the edges of a rectangle are drawn"); // check here
edges.each(function() { // otherwise asserts here do not get called
  ...
}

Pixel comparison / Float calculation checks

Use assert.closeTo in scenarios where we are checking pixels against data values and in float arithmetic.

Measure the size of background() instead of the <svg>

PhantomJS doesn't measure elements in the same way as real browsers: it considers the size of <svg>s based on their content rather than the <svg>'s actual width and height. Append a <rect> and measure that, or measure component.background() instead.

Do not have svg.remove() inside an afterEach() block

That is because if something goes wrong with the test, we want to have the visual.

Use date.getTime() instead of date.valueOf() to compare dates

• Obviously don't use === because that is reference equality
• getTime has the advantage that it explodes for non-dates

Minor

• Use plot.content() instead of plot._renderArea
• Make sure we use the it() semantically. It should read like a sentence. For example it("does not draw rectangles for invalid data points") instead of it("data points that are not valid, do not draw rectangles on the plot")

Suggestions:

These are just suggestions of best practices, slowly move them upwards as people agree. In add proposer & agreers to each to have a sense of what is okay

• Top level describes should act as category delimiters and should not have beforeEach(). That is because sometime we want to add a new, totally unrelated test to some category (say Plots.Line) and the beforeEach() does not make sense. Top Level means the title of the file (so for example "Plots" and "RectanglePlot" will be top level) (@acioara)