Enhancing tests of config parse

[kf6kjg]

See commit messages for reasons behind each change.

These changes are driven by the comments at bcoe#436 (comment)

@mcknasty what do you think of these changes? It's your branch, and your PR that I'm targeting. I figured that when you invited me to collaborate on your repo that you were telling me that if I wanted these changes so much that I should take the effort to put code instead of complaint. :D I'm open to feedback on the changes.

Checklist

• npm test, tests passing
• npm run test:snap (to update the snapshot)
• tests and/or benchmarks are included

[mcknasty]

Initially, just from a glance, this is looking good. I appreciate the help. I am a bit out of pocket at the moment. Had an injury to one of my fingers, while cooking, and typing isn't as easy as I would like. Should be able to download the branch and start the review in about a week.

[kf6kjg]

Sorry to hear! Ya, treat that finger well. Hopefully the food was delicious so that something good came about from the injury, lol. :D

[mcknasty]

Pull Request Review

Thank you so much for your help on this PR. You had a great deal of intelligent comments to make on the project's code review page. With that being said, I have noticed a few improvements in the code you submitted. If you disagree with any of the assessments, please add your notes below mine. Be sure to click the resolve button under each review note, once you feel the issue has been resolved.

Review Branch

I took the liberty of creating a pull request review branch, and you can see the diff here.

Unit Test Martix Report

I also took the liberty of creating a test matrix report. This covers versions of node 14.x - 20.x. This matrix report is produced on Ubuntu Linux. It does not include Mac OS or Windows test results.

Please reach out to me if you need any assistance merging the review branch with the pull request branch.

Linting Errors

The first issue I saw was that the standard linting wasn't passing. If you are not familiar with linting, it's a program that reviews the code for formatting or other obvious errors. It saves the day quite a bit, and it's the first quality assurance check.

To lint the project C8, you need to run the following command from the project's root directory like so:

npx standard

If there are no detected errors, lint will not output any text. If there are errors you can try:

npx standard --fix-errors

Unfortunately, this flag can only fix certain errors. If you have trouble finding or fixing an error, definitely please reach out to me or one of the project's maintainers.

I took the liberty of fixing these errors in the peer review branch.

Unit Test Errors

The second issue I noticed is that one of the unit tests was not passing on Node versions v14.x through v18.x. The specific issue occurred in this block of code. I took the liberty of fixing this issue on the review branch.

I took the liberty of fixing this error in the peer review branch.

The third issue I noticed as it relates to test cases, is there seems to be some extraneous output right before the loadConfigFile describe block

I did not fix this error in the peer review branch.

For the remaining concerns, please see the review notes below.

[mcknasty]

Please take a look at these comments. Remember, I already refactor much of this in the peer review branch.

I still am looking at the test case files and will review those in an additional review, once this one is complete.

[mcknasty]

Question: What does the second parameter di? I assume an array or an object. Does the di stand for dependency injection? I see it called a great deal in the test cases. I am concerned this might get flagged in another peer review.

I did not address in the review branch.

[kf6kjg]

Correct, It's what is known to me as "poor man's Dependency Injection" - its purpose would be clearer in TypeScript, where the signature would look like function loadConfigFile (path: string, _di?: {readFile: (path: string) => string, readJs: (path: string) => string}) { - it's ONLY to be used by the test system to allow stubbing the side-loaded inputs to the function.

I note that while I call it "poor man's DI", it's the ONLY option I've found when it comes to DI for functions in JavaScript. All DI libraries I've found only work with classes.

[kf6kjg]

Since this is JavaScript and not TypeScript we could get away with altering the code to use the following pattern:

function loadConfigFile (path) {
  const di = {
    readFile: (path) => readFileSync(path, 'utf8'),
    readJs: (path) => require(path),
    ...arguments[1] // Second argument is only ever provided as dependency injection from the test suites.
  }

[mcknasty]

Issues with this implementation of Dependency injection aka di.

In most implementations of di, implementation files don't call the constructor. It's written in a general way and implemented in the API. It's decoupled. Overall the di pattern does extend the constructor to add behavior. Very similar to writing a class function and requiring an interface. This extension can cause additional errors and it can be troublesome to debug. It's a free-for-all all on the definition of a class, instead of extending a class definition in another file.

When I was first introduced to the di, my first thought was, "This is an alternative to the factory pattern". Some accuse di of being a golden hammer, which is one of the warnings of the book titled "Design Patterns"

Would it be possible to rework this class function and the test cases without this pattern?

• Edit: 2024-01-31 Fixed a typo. Was referring to a function, not a class. And, yes I do see javascript functions as classes.

const f = function(){}; 
console.log(f instanceof Object); //prints true
console.log(typeof c.prototype) //prints 'object'

[kf6kjg]

Class? There's no class here, only a function. Unless you are conceptualizing JavaScript functions as classes with implicit constructors, which they CAN be, but that concept leads to difficult to maintain code - and often requires a complete rewrite to an actual class when the project matures further.

In this case I simply wanted to be able to inject alternative responses to the side-loaded inputs to this file, the calls to reading from disk. There are two approaches I can see: the first is to do as I've already demonstrated. The second is to add optional parameters, a la

function loadConfigFile (path, readFile = (path) => readFileSync(path, 'utf8'), readJs = require(path)) {

But this means that if I want to write an integration test that needs to override readJs but leave readFile defined with the default I'm stuck as I cannot do that. There's complex ways around that, but the simplest approach is to pass an object, e.g.

function loadConfigFile (path, di = { readFile: (path) => readFileSync(path, 'utf8'), readJs: require(path) }) {

But that has the same problem: if I want to write an integration test that stubs one but not the other I'm stuck. So instead I allow for either to be overridden by using the { defaultValuesHere, ..._di} syntax as I showed in this PR.

As to off-the-shelf DI, let's go down the rabbit hole: let's say we redid this as a class just so we could use off-the-shelf DI. We'd have to create classes that wrap the functionality we are wanting to inject. We'd then have to register them to the DI library, often using JS decorators, and then register the class that wraps loadConfigFile ,again most likely using JS decorators. Oops, JS support for decorators doesn't exist in Node14 IIRC, plus a lot of work to solve a problem when a simpler pattern exists.

[kf6kjg]

To respond directly to your question: Yes, this could be reworked to not pass in alternative implementations of those two functions. There are two approaches I'm familiar with:

1. Use Sinon's ability to mutate the global namespace the functions come from and thus stub any call to require or readFileSync. However mutating global anything in a test suite is an anti-pattern, plus it is explicitly disallowed in ESM. So I no longer use this technique. Plus I'm not certain that stubbing require is even possible or wanted - which would lead to creating another file which implements a function that calls require - a pointless waste of time IMO.
2. Do as you've done elsewhere and create files on disk for it to read. This results in an integration test not a unit test and as a consequence makes a test that should have taken a couple of milliseconds at best take an order or two of magnitude longer due to the IO delay - plus it potentially results in random test failures if the disk has issues. Since other test suites are indeed exercising that path of reading files, I figured I didn't need to write another integration test, I just needed to write a unit test - and to me that means that I have direct programmatic control of every input to the function under test, not indirectly by having it also hit the filesystem.

[mcknasty]

Thank you for all your notes about this concern today. I am looking more into the topic, and I might have a new understanding of pure di or poor man's di. Give me a bit more time to review this topic you presented. I will get back to you soon this next week.

[kf6kjg]

When I started learning about DI myself I read an article that began explaining about the need of DI by using a simple parameter-based injection of a dependency into a function. But the author then immediately jumped to classes and then went to his favorite DI Container tool without giving any other solutions for DI on functions. After a year or so of using various hacks, I found out at least one reason why: all these DI Container systems use decorators, and decorators cannot be used on functions. As I continued to dig I found that a LOT of people were using one-off techniques much like I've done here, though often not as advanced as they've not had to support the use-cases I've met while using this DI pattern daily at work.

[kf6kjg]

Just linking to an easier to look at diff between this PR and the review branch: kf6kjg/c8@enhancing_tests_of_config_parse...mcknasty:c8:enhancing_tests_of_config_parse-review

[kf6kjg]

In response to your code comment on the ConfigParsingError class: The purpose of these error classes is to be very clear in the type of the error what kind of error is being thrown. Thus if we were to change it to something more generic, I'd still argue that a ConfigParsingError that extends the new class would be good API design.

Secondly I use a rule that I create abstractions only if there's a current need, e.g. if I've got multiple classes needing that . This is a corollary to the YAGNI principle.

As an aside, if c8 doesn't expose an API - which it doesn't appear to from a cursory glance at the README - then it can be argued via YAGNI that these error classes and their extra logic could be simply removed in favor of throwing generic Error objects with the relevant strings built at each location.

[mcknasty]

My logic is as follows:

1. ConfigParseError as a noun, doesn't describe what the class represents, rather than how it's being implemented. The class throws another error, if an error has already been thrown. That is why I made the suggestion AppendableError as a class name.
2. You should write all code like you are writing an API. You never know when you, or someone else, might need to extend an interface or a class. Code reuse is at the core of this concept and a good object-oriented programming paradigm. Code reuse leads to fewer unit tests and fewer errors.
3. I do need to rewrite some of the error handling for this project. So yes, I am going to need it. 😃

If you want to make this change out of scope for this release, I am ok with that. Please keep the comments above the class though.

[mcknasty]

There seems to be some extraneous output right before the loadConfigFile describe block

[kf6kjg]

When I named it ConfigParseError I was describing what it represents not what it does or its implementation: as an Error class this represents a class of errors that are being thrown, in this case that there was an error parsing the given configuration. Yes it embeds the original error message, but that's to help the user figure out what went wrong. I could have just thrown the original error, but those errors tend to not be easy to understand why they existed - plus a custom error is easier to handle as an explicit part of the API for a common use case.

As to the "extraneous output" that's the name of the outermost describe block, describe(__filename, () => { describing which test suite file the tests are coming from. I've found that path to be very helpful over time of use as I can easily locate which file a successful test is coming from.

[kf6kjg]

GitHub is refusing to let me reopen this, should I rebuild it?

[mcknasty]

GitHub is refusing to let me reopen this, should I rebuild it?

We might need to need to. Let me reopen the PR on the main project first.

[mcknasty]

Here is the reference to the new PR

[kf6kjg]

Rebuilt this PR at #8