Skip to content

Commit

Permalink
fix: bug with assert.equal() where one of the args is null and the ot…
Browse files Browse the repository at this point in the history
…her is an object due to JS bug with typeof null === 'object'

- also elaborated on README about before() and after() anti-pattern
  • Loading branch information
mhweiner committed Apr 1, 2024
1 parent 3c94a49 commit 1c945c2
Show file tree
Hide file tree
Showing 12 changed files with 150 additions and 20 deletions.
2 changes: 1 addition & 1 deletion .c8rc.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"**/*.spec.ts",
"**/*.spec.js"
],
"reporter": [],
"reporter": [],
"all": true,
"cache": false,
"clean": true
Expand Down
11 changes: 3 additions & 8 deletions .github/workflows/workflow.yml
Original file line number Diff line number Diff line change
@@ -1,21 +1,16 @@
name: Release

on:
push:
branches: [master, beta]

jobs:
release:
name: Release
runs-on: ubuntu-latest

steps:
- name: Checkout
uses: actions/checkout@v2
- name: Setup Node.js
uses: actions/setup-node@v1
- uses: actions/checkout@v3
- uses: actions/setup-node@v4
with:
node-version: 14.x
node-version: latest
- name: Install dependencies
run: npm ci
- name: Lint
Expand Down
33 changes: 30 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,12 @@ A simple, fast, opinionated, and defensive Typescript/Javascript test runner des
- Errors or unhandled promise rejections are buffered and grouped under the test file in the output. This helps you know where they came from.
- Clear documentation.

**Opinionated 🎓**
- No nesting of tests. This has the following benefits:
**Opinionated for Quality 🎓**
- No deep nesting of tests. This has the following benefits:
- Pressures programmers to break apart their code into smaller pieces.
- Test code becomes much simpler and easier to read and maintain.
- Output reporting becomes much simpler and easier to read and understand.
- No built-in `before()` and `after()`. This leads to messy design patterns and mistakes in test code. Most tests shouldn't require teardowns. Of course, you could still create your own.
- No built-in `before()` and `after()`. [This is a common anti-pattern.](#why-no-built-in-setup-and-teardown)

**Modern Language Features ✨**
- Async/Await/Promise Support
Expand Down Expand Up @@ -209,6 +209,33 @@ Inspiration was also taken from the following:
- [Rethinking Unit Test Assertions](https://medium.com/javascript-scene/rethinking-unit-test-assertions-55f59358253f) by [Eric Elliot](https://medium.com/@_ericelliott).
- Other test frameworks [tape](https://github.com/substack/tape), [AVA](https://github.com/avajs/ava) and [node-tap](https://github.com/tapjs/node-tap).
# Why no built-in setup and teardown?
The use of setup() and teardown() or before() and after() methods in unit tests is often considered an anti-pattern [[1](https://jamesnewkirk.typepad.com/posts/2007/09/why-you-should-.html), [2](https://stackoverflow.com/questions/1087317/do-setup-teardown-hurt-test-maintainability), [3](https://medium.com/@jameskbride/testing-anti-patterns-b5ffc1612b8b)]. These methods are typically used to set up and tear down the test environment before and after each test case. While they may seem convenient at first, they can lead to several issues:
- Unit tests should be idempotent and non-destructive. They should not require a setup or teardown in the first place. If you're using one, it probably means you're running an integration test and doing stateful things, such as touching a database or changing a file on disk. (If you do intened to write an integration test, keep reading, we'll address this.)
- Hidden dependencies: When using setup() and teardown() methods, the dependencies between test cases are not explicit. This can make it difficult to understand and reason about the test suite as a whole. It becomes harder to identify which tests rely on specific setup steps and which tests may be affected by changes in the teardown process.
- Test pollution: setup() and teardown() methods can introduce test pollution, where changes made in one test case affect the outcome of subsequent test cases. This can lead to unreliable and unpredictable test results, making it harder to isolate and identify the root cause of failures.
- Complexity and maintenance: As the test suite grows, the complexity of managing the setup and teardown methods increases. It becomes harder to ensure that the test environment is properly set up and cleaned up for each test case. This can result in fragile tests that are difficult to maintain and modify.
Instead of using setup() and teardown() or before() and after() methods, it is recommended to follow the principles of Arrange-Act-Assert (AAA) in unit testing. Each test case should explicitly set up the necessary preconditions, perform the actions being tested, and assert the expected outcomes. This approach makes the dependencies between test cases explicit and promotes better test isolation and maintainability.
By avoiding the use of setup() and teardown() or before() and after() methods, you can write more focused and reliable unit tests that are easier to understand, maintain, and debug.
If you still need a re-useable function, such as for your integration tests, you can of course just write your own. It's just a function, after all! Why should a framework provide that?
```
function setup() {
// do your dirty deeds
}
function teardown() {
// do your dirty deeds
}
```
# Ways to Contribute
- Submit an [issue](https://github.com/mhweiner/hoare/issues) with your feature request or bug report
Expand Down
2 changes: 1 addition & 1 deletion demo/greet.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {test} from '../src';
import {test} from '../src'; // from 'hoare'
import {greet} from './greet';

test('returns expected object', (assert) => {
Expand Down
2 changes: 1 addition & 1 deletion demo/helloworld-js.spec.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* eslint-disable @typescript-eslint/no-require-imports, @typescript-eslint/no-var-requires */

const test = require('../dist/test').test;
const test = require('../dist/test').test; // require('hoare').test
const helloworld = require('./helloworld-js');

test('returns "hello, world"', (assert) => {
Expand Down
2 changes: 1 addition & 1 deletion demo/helloworld.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {test} from '../src';
import {test} from '../src'; // from 'hoare'
import {helloworld} from './helloworld';

test('returns "hello, world"', (assert) => {
Expand Down
2 changes: 1 addition & 1 deletion demo/isValidWord.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {test} from '../src';
import {test} from '../src'; // from 'hoare'
import {mock} from 'cjs-mock';
import * as mod from './isValidWord'; // just used for type

Expand Down
2 changes: 1 addition & 1 deletion demo/validate.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {test} from '../src';
import {test} from '../src'; // from 'hoare'
import {InvalidPhoneNumber, validatePhoneNumber} from './validate';

test('valid phone number', (assert) => {
Expand Down
2 changes: 1 addition & 1 deletion src/assertions/equal.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {Assertion} from '../test';
import {deepStrictEqual} from '../deepStrictEqual';
import {deepStrictEqual} from '../lib/deepStrictEqual';
import {diff as diffObj} from 'deep-object-diff';
import {diffChars as diffStr} from 'diff';
import * as util from 'util';
Expand Down
2 changes: 1 addition & 1 deletion src/assertions/errorsEquivalent.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {serializeError} from 'serialize-error';
import {Assertion} from '../test';
import {deepStrictEqual} from '../deepStrictEqual';
import {deepStrictEqual} from '../lib/deepStrictEqual';

export function errorsEquivalent(assertions: Assertion[], actualErr: any, expectedErr: any, description?: string) {

Expand Down
107 changes: 107 additions & 0 deletions src/lib/deepStrictEqual.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
import {test} from '../test';
import {deepStrictEqual} from './deepStrictEqual';

test('deepStrictEqual() returns true for primitives that are strictly equal', (assert) => {

assert.equal(deepStrictEqual(1, 1), true);
assert.equal(deepStrictEqual('a', 'a'), true);
assert.equal(deepStrictEqual(true, true), true);
assert.equal(deepStrictEqual(null, null), true);
assert.equal(deepStrictEqual(undefined, undefined), true);

});

test('deepStrictEqual() returns false for primitives that are not strictly equal', (assert) => {

assert.equal(deepStrictEqual(1, 2), false);
assert.equal(deepStrictEqual('a', 'b'), false);
assert.equal(deepStrictEqual(true, false), false);
assert.equal(deepStrictEqual(null, undefined), false);

});

test('deepStrictEqual() returns false if one is a non-object and the other is an object', (assert) => {

assert.equal(deepStrictEqual(undefined, {a: {b: '2'}}), false);
assert.equal(deepStrictEqual({a: {b: '2'}}, undefined), false);
assert.equal(deepStrictEqual(1, {a: {b: '2'}}), false);
assert.equal(deepStrictEqual({a: {b: '2'}}, 1), false);

});

test('deepStrictEqual() returns false if one is null (null is an object due to JS bug)', (assert) => {

assert.equal(deepStrictEqual(null, {a: {b: '2'}}), false);
assert.equal(deepStrictEqual({a: {b: '2'}}, null), false);
assert.equal(deepStrictEqual(null, true), false);
assert.equal(deepStrictEqual(true, null), false);
assert.equal(deepStrictEqual(null, 'a'), false);
assert.equal(deepStrictEqual('a', null), false);

});

test('deepStrictEqual() returns true for objects that are deeply equal', (assert) => {

assert.equal(deepStrictEqual({a: 1, b: 2}, {a: 1, b: 2}), true);
assert.equal(deepStrictEqual({a: {b: 2}}, {a: {b: 2}}), true);
assert.equal(deepStrictEqual({a: {b: {c: 3}}}, {a: {b: {c: 3}}}), true);

});

test('deepStrictEqual() returns false for objects that are not deeply equal', (assert) => {

assert.equal(deepStrictEqual({a: 1, b: 2}, {a: 1, b: 3}), false);
assert.equal(deepStrictEqual({a: {b: 2}}, {a: {b: 3}}), false);
assert.equal(deepStrictEqual({a: {b: {c: 3}}}, {a: {b: {c: 4}}}), false);

});

test('deepStrictEqual() returns false for objects that have different numbers of keys', (assert) => {

assert.equal(deepStrictEqual({a: 1, b: 2}, {a: 1}), false);
assert.equal(deepStrictEqual({a: 1}, {a: 1, b: 2}), false);

});

test('deepStrictEqual() returns false for objects with different keys', (assert) => {

assert.equal(deepStrictEqual({a: 1, b: 2}, {a: 1, c: 3}), false);

});

test('deepStrictEqual() returns false for objects with different values', (assert) => {

assert.equal(deepStrictEqual({a: 1, b: 2}, {a: 1, b: 3}), false);

});

test('deepStrictEqual() returns false for objects with different types', (assert) => {

assert.equal(deepStrictEqual({a: 1, b: 2}, {a: 1, b: '2'}), false);

});

test('deepStrictEqual() returns false for objects with different nested values', (assert) => {

assert.equal(deepStrictEqual({a: {b: 2}}, {a: {b: 3}}), false);

});

test('deepStrictEqual() returns false for objects with different nested types', (assert) => {

assert.equal(deepStrictEqual({a: {b: 2}}, {a: {b: '2'}}), false);

});

test('deepStrictEqual() returns false for objects with different nested numbers of keys', (assert) => {

assert.equal(deepStrictEqual({a: {b: 2}}, {a: {b: 2, c: 3}}), false);
assert.equal(deepStrictEqual({a: {b: 2, c: 3}}, {a: {b: 2}}), false);

});

test('deepStrictEqual() returns false for objects with different nested keys', (assert) => {

assert.equal(deepStrictEqual({a: {b: 2}}, {a: {c: 3}}), false);

});
3 changes: 2 additions & 1 deletion src/deepStrictEqual.ts → src/lib/deepStrictEqual.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ export function deepStrictEqual(obj1: any, obj2: any) {

if (obj1 === obj2) return true;
if (isPrimitive(obj1) && isPrimitive(obj2)) return obj1 === obj2;
if (typeof obj1 !== typeof obj2) return false;
if (typeof obj1 !== typeof obj2) return false; // will not work for null due to JS bug (https://alexanderell.is/posts/typeof-null/)
if (obj1 === null || obj2 === null) return false; // if both ARE null, we already returned true
if (Object.keys(obj1).length !== Object.keys(obj2).length) return false;

// compare objects with same number of keys
Expand Down

0 comments on commit 1c945c2

Please sign in to comment.