Discussion on: Test Driven Development 101 and JS testing

[Ameet Kaustav]

Hi @leanminmachine ,
First of all, congratulations on your first post in dev.to. Your article is definitely inspiring.

However, I found a few potential flaws in your article which got me a bit confused. Please consider the following as positive feedback to improve the article for future readers - I do not aim to discredit your work. Feel free to accept/reject each proposal.

1. Arabic vs Roman Numerals

Arabic Numerals are: 0, 1, 2, 3, 4, ...

Roman Numerals are: N, I, II, III, IV, ...

In your jest based test file, the function being tested is named arabicToRomanNumbers. From the name, I infer your function takes in an Arabic numeral (e.g. 1) and converts it into a Roman Numeral (e.g. I). However, in your test case, you seem to be passing in a roman numeral (I) and expecting an Arabic numeral (1). That seemed confusing to me.

My proposal: Maybe change the function name to romanToArabicNumbers. Or modify your test case to accept 1 and return I.

2. module.exports vs exports.module

In your main JS file, the following line appears at the end of the file:

export.modules = arabicNumber;

I DO NOT believe that there is a global export.modules object available in NodeJS or in ES5/6/7.

My proposal: If you're writing your file for NodeJS, consider changing to

module.exports = arabicNumber;

or simply

exports = arabicNumber;

Otherwise, if you're writing your file for ES6 style exports, consider changing to the arabicNumber function definition to the following.

export default function arabicNumber(...) {
  ...
}

Like @kayis pointed out, you may also be able to use the experimental ECMAScript Module (ESM) style exports in NodeJS 10.0 or above with the .mjs extension on your main file. Read more here.

3. Inconsistent Function Names

From the require statement in the test file, I assumed the main file is named arabicToRomanNumbers.js. It assumes that the default exported member is a function.

However, inside the main JS file, the default exported function is named arabicNumber. It does not match the function which you are trying to test in your jest test file.

My proposal: While this is not a big deal, you may want to consider keeping function names references consistent throughout your article. It really helps readers new to the Javascript module system.

4. Possible Typo in Code

Your main arabicNumber function contains few flaws:

1. It's missing the closing braces for the function body.
2. It compares a string 1 and returns numeric 1. Perhaps you should compare with string I.
3. In my opinion, the parameter name could be improved.

Your code:

function arabicNumber(string) { // 3. bad name for the parameter
if (string == “1”) { // 2. Possible typo
return 1;
} // 1. Missing closing braces for function

My proposal: You might want to consider doing something like the following (the logic in this example is bad - and can be improved, but it helps with your test case):

function arabicNumber(roman) {
  let arabic = NaN;

  // Logic can be improved - for demo purposes only
  if (roman === 'I') {
    arabic = 1;
  } else if (roman === 'II') {
    arabic = 2;
  } else ...

  return arabic;
}

@akaustav

[leanminmachine]

Thank you for the comment & clarification!

I didn't expect this to blow up publicly though so I didn't really scrutinise it before posting, just wanted to post on here to keep track of what I've learnt so far :D Probably I'll edit it, and repost again :)

[Ameet Kaustav]

Great. Looking forward to your edits. Good luck!