Collection of unit test best practices

[mokimo]

We should collect best practices for writing web test runner unit tests and highlight how to fix bad patterns. This thread should serve a good entry point for anyone, to provide a blueprint on how to avoid certain patterns.

[mokimo]

Bad: Loading external resources via fetch and XMLHttpRequest

Loading external resources is bad for several reasons.

• Resource could be deleted or moved -> Test starts failing or logging errors
• Resource could sometimes be available depending on the network connection. -> If it's not? Test fails.
• Loading 3rd party resources takes up bandwidth and potentially incurs extra costs.
• If you wait for a resource to load, your tests might be slower than they should be.

The following examples shows a bad pattern and then how to fix the issue, by replacing the global window.fetch with a stub. XMLHttpRequest request would work in the same way.

Bad
NOT mocking requests

import { expect } from 'chai';
import { fetchData } from './myModule.js';

describe('fetchData', () => {
  it('fetches data from external API', async () => {
    // api.external.com/data is called - we cannot trust that the API is always available.
    const data = await fetchData('https://api.external.com/data');
    expect(data).to.not.be.null;
    expect(data).to.be.an('array');
    expect(data).to.not.be.empty;
  });
});

Good
mocking requests

import { expect } from 'chai';
import sinon from 'sinon';
import { fetchData } from './myModule.js';

const mockRes = ({ payload, status = 200, ok = true } = {}) => new Promise((resolve) => {
  resolve({
    status,
    ok,
    json: () => payload,
    text: () => payload,
  });
});

let originalFetch = window.fetch
describe('fetchData', () => {
  beforeEach(() => {
    window.fetch = sinon.stub().callsFake(() => mockRes({}));
  });

  afterEach(() => {
    // Do not build up any test state - reset window.fetch to it's original state
    window.fetch = originalFetch;
  });

  it('fetches data from external API', async () => {
    const payload = [{ id: 1, name: 'Test' }];

    window.fetch.returns(mockRes({ payload }));
    
    // As we stubbed the requests, we will always get a reliable result.
    // in this example the fetchData module would internally use fetch, which is replaced by the stub.
    const data = await fetchData('https://api.external.com/data');

    expect(data).to.not.be.null;
    expect(data).to.be.an('array');
    expect(data).to.not.be.empty;
    expect(data).to.deep.equal(payload);
    
    expect(window.fetch.calledWith('https://api.external.com/data')).to.be.true;
  });
});

How to spot this pattern?

The easiest way is running your tests in isolation, opening the browser, and filtering all the domains that point to localhost
Making requests is not the end of the world and sometimes u can't avoid them e.g. for iFrames that are loading external embeds - but your tests should never depend on the result of any request.

Another way is looking for 404 network requests in the test logs or a warning ** fetch request for an external resource is disallowed in unit tests

Real world example

Here is an example of a good test for caas that properly implements this pattern in the milo codebase. https://github.com/adobecom/milo/blob/31e2b2ce6eff62844ab735bb11ea11efd8d54efa/test/blocks/caas/caas.test.html

[mokimo]

Bad: Loading external scripts

Like loading external resources via fetch / XMLHttpRequest, this is bad for several reasons. Refer to the post above to get more insight.

Usually const { loadScript, loadStyle } from "./libs/utils/utils.js" are common for loading external scripts.

Bad

// blocks/instagram/instagram.js
import { createIntersectionObserver, createTag, loadScript, isInTextNode } from '../../utils/utils.js';
export default function init(a) {
  if (isInTextNode(a)) return;
  const embedInstagram = async () => {
    const anchor = createTag('a', { href: a.href });
    const blockquote = createTag('blockquote', { class: 'instagram-media', 'data-instgrm-captioned': '' }, anchor);
    const wrapper = createTag('div', { class: 'embed-instagram' }, blockquote);
    a.parentElement.replaceChild(wrapper, a);
    loadScript('https://www.instagram.com/embed.js');
  };
  createIntersectionObserver({ el: a, callback: embedInstagram });
}

// instagram.test.js
import { readFile } from '@web/test-runner-commands';
import { expect } from '@esm-bundle/chai';
import { waitForElement } from '../../helpers/waitfor.js';
const { default: init } = await import('../../../libs/blocks/instagram/instagram.js');
document.body.innerHTML = await readFile({ path: './mocks/body.html' });
describe('instagram', () => {
  it('renders embed post for instagram link', async () => {
    const instagram = document.querySelector('a');
    init(instagram);
    // the await is needed as https://www.instagram.com/embed.js is being loaded
    // if the script times out, the test will fail
    const iframe = await waitForElement('iframe');
    const blockquote = document.querySelector('.instagram-media');
    const wrapper = document.querySelector('.embed-instagram');
    expect(blockquote).to.not.be.null;
    expect(wrapper).to.not.be.null;
    expect(iframe).to.be.exist;
  });
});

Good

Web test runner allows using import maps with import maps we can mock the milo utils and ensure that loadScript is a stub instead of the real function.

// embed-utils.js
// when mocking ensure all the functions that are used are mocked.
// ....
import { stub } from 'sinon';
export function isInTextNode() {
  return false;
}
export const loadStyle = stub();
export const loadScript = stub();
export function createIntersectionObserver({ el, callback /* , once = true, options = {} */ }) {
  // fire immediately
  callback(el, { target: el });
}

// instagram.test.html
<html>
  <head>
    <script type="importmap">
      {
        "imports": {
          // This ensure that another file is loaded instead of utils.js
          // Depending on the nature of the script, you can also add other import maps
          // Or other scripts that load the 3rd party script from localhost reliably.
          "../../../libs/utils/utils.js": "./mocks/embed-utils.js"
        }
      }
    </script>
  </head>
  <body>
    <a href="https://www.instagram.com/p/Cl1xhZuvN8N/?utm_source=ig_web_button_share_sheet">https://www.instagram.com/p/Cl1xhZuvN8N/?utm_source=ig_web_button_share_sheet</a>
  </body>
  <script type="module">
    import { runTests } from '@web/test-runner-mocha';
    import { readFile } from '@web/test-runner-commands';
    import { expect } from '@esm-bundle/chai';
    import { stub } from 'sinon';
    import { waitForElement } from '../../helpers/waitfor.js';
    import { loadScript } from '../../../libs/utils/utils.js';
    import init from "../../../libs/blocks/instagram/instagram.js"

    runTests(() => {
      describe('instagram', () => {
        it('renders embed post for instagram link', () => {
          const instagram = document.querySelector('a');

          // We don't need to wait for the script to load
          init(instagram);
          const blockquote = document.querySelector('.instagram-media');
          const wrapper = document.querySelector('.embed-instagram');

          // We can also test that the script would have been loaded
          // and assume instagram *would* have been rendered.
          expect(loadScript.callCount).to.equal(1);
          expect(blockquote).to.not.be.null;
          expect(wrapper).to.not.be.null;
        });
      });
    });
  </script>
</html>

Real world example

Here is an example of a good test for caas that properly implements this pattern in the milo codebase. Unl https://github.com/adobecom/milo/blob/31e2b2ce6eff62844ab735bb11ea11efd8d54efa/test/blocks/caas/caas.test.html

Another example that migrated some tests that were loading external scripts: #701

[meganthecoder]

This doesn't appear to work for dynamic imports (at least not in consumer repos). Is there an alternative in that case?

const miloLibs = getLibs();
const { getMetadata } = await import(`${miloLibs}/utils/utils.js`);

[overmyheadandbody]

I think getLibs was a generic way of showcasing the principle. For consumer projects, I'd say you can try calling getConfig() and use the base property. I had a quick look on https://main--bacom--adobecom.hlx.page/customer-success-stories and the base leads to the Milo libs folder.

[meganthecoder]

It's not that the libs domain is incorrect. In consumer repos, getLibs is the right way to get the milo libs. It's that the import map doesn't recognize the output of (`${miloLibs}/utils/utils.js`) as the same as https://main--milo--adobecom.hlx.live/libs/utils/utils.js (what I put in the import map).

Here's my branch: https://github.com/adobecom/bacom-blog/tree/methomas/article-tests

When I debug the test in the browser, I see that the milo version of the utils.js file is being imported, not the mocks/scripts-utils.js file specified in the import map. If I change the import statement to use the hardcoded url (commented out in the example), then it works.

In test/scripts/utils.test.html:

<script type="importmap">
      {
        "imports": {
          "https://main--milo--adobecom.hlx.live/libs/utils/utils.js": "./mocks/scripts-utils.js"
        }
      }
</script>
...
<script type="module">
...
    import { setLibs, buildAutoBlocks } from '../../scripts/utils.js';
...
    it('builds the article header block', async () => {
          document.body.innerHTML = await readFile({ path: './mocks/articleHeaderBody.html' });
          await buildAutoBlocks();
          const articleHeader = await waitForElement('.article-header');
          expect(articleHeader).to.exist;
    });

In scripts/utils.js:

export async function buildAutoBlocks() {
  const miloLibs = getLibs();
  const { getMetadata } = await import(`${miloLibs}/utils/utils.js`);
  // const { getMetadata } = await import('https://main--milo--adobecom.hlx.live/libs/utils/utils.js');
  ...
}

[mokimo]

@meganthecoder Weird, seems to be a limitation with dynamically importing a module + having a dynamic path set by miloLibs

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script/type/importmap
Import maps allow developers to specify (almost) any text they want in the module specifier; the map provides a corresponding value that will replace the text when the module specifier is resolved.

The way I read that, is that it does an exact string match from the import and replaces the string as the module is resolved.
So when you import scripts/utils.js it can't replace import('${miloLibs}/utils/utils.js'); as the value does not exist yet. For https://main--milo--adobecom.hlx.live/libs/utils/utils.js it replaces it with the given import map

Two workarounds you could use

1. Use an import map to mock scripts/utils.js instead
2. Set libs to load from localhost if possible

[meganthecoder]

Following up on this in case it helps others. I had success setting libs to load from localhost. The example is from: bacom blog

// In: /test/scripts/utils.test.js
describe('Auto Blocks', () => {
  before(() => {
    setLibs('/test/scripts/mocks', { hostname: 'none', search: '' }); // Set libs to load from localhost
  });
  ...
  it('builds the article header block', async () => {
    await buildAutoBlocks(); // This is the function that calls external scripts
    expect(document.querySelector('.article-header')).to.exist;
  });
 });
 
 // In: /blog/scripts/utils.js
export async function buildAutoBlocks() {
  const miloLibs = getLibs();
  const { getMetadata } = await import(`${miloLibs}/utils/utils.js`); // This is the external script we're trying to mock
  ...
}

// In: /test/scripts/mocks/utils/utils.js
export function getMetadata(name, doc = document) { // This is the mocked function that's called
  ...
}

[mokimo]

Good: Avoiding 404 error logs

404s are leading to noise and are not necessary. Ensure that you either mock the resources, or don't load them if they aren't needed for a test.

Bad
The mock will expect ./media_109b289ba2841014b405e8cda9dbeb3e6f7471201.jpeg to exist. If you want to test any functionality that renders this image, you can ensure that is really exists. Otherwise remove it!

// Bad mock data copied over from production
<p>
  <picture>
    <source type="image/webp" srcset="./media_109b289ba2841014b405e8cda9dbeb3e6f7471201.jpeg?width=2000&amp;format=webply&amp;optimize=medium" media="(min-width: 400px)">
    <source type="image/webp" srcset="./media_109b289ba2841014b405e8cda9dbeb3e6f7471201.jpeg?width=750&amp;format=webply&amp;optimize=medium">
    <source type="image/jpeg" srcset="./media_109b289ba2841014b405e8cda9dbeb3e6f7471201.jpeg?width=2000&amp;format=jpeg&amp;optimize=medium" media="(min-width: 400px)">
    <img loading="eager" alt="" type="image/jpeg" src="./media_109b289ba2841014b405e8cda9dbeb3e6f7471201.jpeg?width=750&amp;format=jpeg&amp;optimize=medium" width="2000" height="875">
  </picture>
</p>

Good

// Better mock data that doesn't load the resources.
<picture>
  <source type="image/webp" srcset="" media="(min-width: 400px)">
  <source type="image/webp" srcset="">
  <source type="image/jpeg" srcset="" media="(min-width: 400px)">
  <img loading="eager" alt="" type="image/jpeg" src="" width="2000" height="875">
</picture>

[mokimo]

Bad: Building up test state from other tests

Tests that share state are heavily discouraged! They are very tedious to debug and can lead to unexpected behavior. Starting "fresh" and setting state in every beforeEach and resetting state in every afterEach block provides better results.

Generally, if you put a it.only on any test, it should always run. If your tests don't run with/without other tests, that's a code smell

Bad

describe('example of a bad test!!', () => {
  // only setting a 'before' once
  before(() => {
    document.body.innerHTML = "Text"
  })

  it('test one modifies some state, either on the DOM or within a class, or anything shared.', () => {
    expect(document.body.innerHTML).to. Equal("Text")
    // modifying "shared" state
    document.body.innerHTML = "Text2"
  })

  it('depends on the state of the previous test', () => {
    // depend on the state another test (no before/beforeEach) set .. very bad!
    expect(document.body.innerHTML).to.equal("Text2")
  })
})

Correct

describe('example of a good test', () => {
  // beforeEach ensures the "shared" DOM state is always predictable in its base state before each test
  beforeEach(() => {
    document.body.innerHTML = "Text"
  })

  it('This test does not modify anything.', () => {
    // At the end of this test, we know that the beforeEach will reset body.innerHTML. 
    expect(document.body.innerHTML).to.equal("Text")
    document.body.innerHTML = "something else"
    expect(document.body.innerHTML).to.equal("something else")
  })

  it('Does not depend on whatever test 1 set.', () => {
    document.body.innerHTML = "Text2"
    expect(document.body.innerHTML).to.equal("Text2")
  })
})

Real World Example

Refactoring the util tests to properly reset the head & body before each test using beforeEach instead of before

Tests were relying that the DOM had been modified in a specific way, by a previous test. As tests can run in parallel, when the DOM wasn't modified by a previous test - the test would sometimes fail leading to issues with sporadically failing tests on the CI.

[JasonHowellSlavin]

@mokimo this is a great collection of best practices already. How granular did you want this discussion to get? I have had a few items I often will add during my Code Reviews that are a bit nit-picky, but I feel strongly about, for example:

Ensuring it blocks start with verbs:

describe('My block', () => {
    it('adds text to a button', () => {})
    it('modifies the structure based on metadata', () => {})
})

Where applicable, adhering to Testing Library's methodology of testing: using our expects to mimic what users would check as opposed to other checks (utilizing text, aria-attributes, etc). You show this good practice by checking the text in your example in "Building up test state..." above.

// Bad 
describe(`Toggle button`, ()=> {
    it('adds the class to the previous toggle', () => {
        toggle.click();
        expect(toggle.classList.contains('open')).to.be.true;
    })
})

// Good
describe(`Toggle button`, ()=> {
    it('adds the class to the previous toggle', () => {
        toggle.click();
        expect(toggle.ariaExpanded).to.equal('true);
    })
})

What do you think?

[mokimo]

Anything that helps long term maintenance of milo tests, so anyone can add whatever seems fitting.

For the nit-pick stuff - that can make sense, as it makes it easier to give context and link to the discussion in a PR rather than making up examples for each PR or make suggestions without the why

[auniverseaway]

LOVE THIS.

[chrischrischris]

We also have a few test utilites that can be useful:

waitfor.js:

• waitForElement : wait for an element to be added to the page
• waitForUpdate : wait for an element to update
• waitForRemoval : wait for an element to be removed
• delay : awaitable promise base delay

selectors.js:

• findByLabel
• findByHeading

generalHelpers.js

• mockFetch : easily mock a fetch request and response

If you find yourself creating a similar function, be sure to add it to the helpers dir.

[overmyheadandbody]

This is quite valuable to everyone IMO, so I'm wondering whether a Unit Testing section could be added to https://milo.adobe.com/docs/engineering/ (or maybe https://github.com/adobecom/milo/wiki) with the principles outlined here. This might provide more visibility over time, as this thread will be pushed out of view.

[mokimo]

Added it to the wiki:

• https://github.com/adobecom/milo/wiki/Resources
• https://github.com/adobecom/milo/wiki/The-Developer-Kitchen-Sink#testing-milo

I wonder if there's any other places we should add this to?

[honstar]

I second this request 👍 For now, I've pinned this discussion in the General category.