Alarm Clock Project TDD revision

[SallyMcGrath]

Which module(s) and week(s) does this change affect? Module(s): JS2 Week(s): 3

What is the work that needs to be done?

Revise the Alarm Clock project tests to use require and exports in order to avoid mocking the DOM

Why is this work important to do?

Ali needs to experiment and come back with Answers

https://syllabus.codeyourfuture.io/js-core-2/week-3/homework/#3-javascript-challenges-14-hours https://github.com/CodeYourFuture/JavaScript-Core-2-Coursework-Week3/tree/main/mandatory/1-alarmclock

Who might need to know about this change?

[40thieves]

I've finally had time to look into this (and remember what exactly I was thinking...)

I think the tl;dr is that my ideas are more difficult to implement that I'd hoped, and so I don't think it's worth the tradeoff for changing it now.

Idea 1: Export some functions to unit test

Technically, this is @illicitonion's idea... https://github.com/CodeYourFuture/JavaScript-Core-2-Coursework-Week3/pull/67#pullrequestreview-687923358. However in this context, I was less interested in semantics of types of tests but more if this sort of test is technically feasible.

To be able to unit test a specific helper function, we would need to export the function to the test in some way. To be able to export a function, we need a module system: either ESM or CJS (thanks Javascript!). Something like this:

// alarmclock.js
export function formatTime(timeInSeconds) {
  ...
}

or

// alarmclock.js
function formatTime(timeInSeconds) {
  ...
}

module.exports = {
  formatTime,
};

Then in the tests:

// alarmclock.test.js
const { formatTime } = require("./alarmclock");

test("Formats time correctly", () => {
  expect(formatTime(19)).toBe("00:19");
});

Unfortunately doesn't really work out, as there's not (yet) a single module system that will work in all "environments".

If we use ESM, then Jest blows up with unexpected syntax. I tried a bunch of things from this docs page, and didn't get very far. I suppose it's possible that someone with more Jest/Node knowledge than me might be able to wrangle everything to get it working, but I would worry about trainees installing older incompatible versions of Node. I think the simpler option here would be to just wait until support in Node & Jest is better.

If we use CJS, then we'd have to include some kind of bundler (webpack/rollup/parcel/etc) to get the app working in the browser. Otherwise the browser will blow up with an module is not defined error. This also feels like a footgun for our trainees.

Idea 2: Switch the test to import directly

Currently the test loads the alarmclock.js file via JSDOM: https://github.com/CodeYourFuture/JavaScript-Core-2-Coursework-Week3/blob/9beb2e131fd9927f00dfd87d6a3feaac4a496211/mandatory/1-alarmclock/alarmclock.test.js#L11-L14

Effectively, we're treating JSDOM like a browser and loading the index.html file, which in turn loads alarmclock.js. This also explains why the runScripts: "dangerously" option is required. We could mock the DOM in the test, and then import alarmclock.js directly. It would something like this:

test("should set heading when button is clicked", () => {
  document.body.innerHTML = `<div>...</div>`;

  // Note: this requires some minor tweaks to alarmclock.js
  require("./alarmclock");

  const heading = page.window.document.querySelector("#timeRemaining");
  const input = page.window.document.querySelector("#alarmSet");
  const button = page.window.document.querySelector("#set");

  input.value = "19";
  button.click();

  expect(heading).toHaveTextContent("Time Remaining: 00:19");
});

IMO, the current approach is slightly unusual. I suspect not many volunteers would know how this works, which raises the maintenance burden. The Jest docs recommend the DOM mocking/direct import approach for testing jQuery: https://jestjs.io/docs/tutorial-jquery.

However, given the points above, we'd only be able to import the entire file and run things via side-effects. Therefore I don't think it's particularly worth the hassle.

I'll leave this open for follow up comment, but I'd lean towards closing as our current approach seems to work fine.

[40thieves]

A bit off-topic, but fwiw I still think it's worth teaching module systems to some extent: it would be nice if we could cover it before trainees get to React.

[40thieves]

Closed this out as there's no action required. ESM need to sort out their 😡❗️🌪 before we can reliably use them across Node & browsers.