Adding a test in FilerJS for fs.promises.rename()

FilerJS: adding a simple test and reviewing a pull request:

FilerJS is an open source Javascript library that acts as an adapter to allow you to interact with a web browser's IndexedDB or WebSQL APIs to make them behave like a Unix style filesystem. Filer itself mimics the function definitions of the fs module of Node.js (which actually modifies files on a server).

While the concept of creating an entire library that essentially one big adapter is fascinating in terms of applying design patterns to functional Javascript programming, I currently have little idea how any of this works. In this case, the best approach to become acquainted with the Filer project  was to look through the code and try to expand its unit tests. At the time of writing, I and y classmates were informed that Filer had recently had methods added to mimic Node's fs.promises tests, but no unit tests we added for these new functions.

I started simple by creating an  issue to add a test to check that fs.promises.rename() was able to rename an existing file object, and add it

Determining what to write:

Determining what logic to use for the test was relatively simple because I could start by looking for the existing test for fs.rename()--the async version of the file rename function that operated with callbacks instead of promises.
After a brief search through the Github repo, I found the file that held the async tests for rename() in tests/spec/fs.rename.spec.js.

Already written test code for fs.rename() of an existing file:

First attempt

The code change can be seen in the Pull request.

One approach to modifying existing code is to modify as little as possible. For this reason, my first attempt at writing a rest for fs.promises.rename() was to just copy existing logic, and replace the fs.rename() call with fs.promises.rename(). This produced code that was easy to write and likely to work because it used the async fs.open() and fs.close() in creating a new file object, and fs.stat() to check its name after renaming rather than the promise versions, which didn't have tests written for them yet. However this produced the normal jumble of callbacks that promises were meant to remove. It also meant that the new promises functions were not being used to create (open() and close()) and check (stat()) the file objects used in the unit test. using the old async functions instead of the promise functions also made the block of code harder to identify as one that tests fs.promises.rename(). Unsurprisingly, this attempt was requested to be changed to include the promise versions of open() close() and stat().
The logic for the test is to create an empty file object with fs.open() and the 'w+' flag, then close it, try to rename it and ensure that it doesn't exist under its old name, but does under the new name.

First attempt: A test as similar to the async test as possible

Second attempt

The response to my initial pull request was that I should replace all async fs functions with fs.promises functions instead for the sake of clarity, and to use Promises.all() to run several promises together. Being unfamiliar with Promise.all(), I found the MDN documentation for Promise.all() to be a bit vague in whether the promises passed to it were all launched to run simultaneously--which would be bad since a file needs to be created before attempting to rename it, and renamed before checking it with fs.promises.stat()-- or if they were run in sequence. A quick check of the MDN's article on promises in general and the article it linked to on using promises didn't make things too obvious either, but it did talk about promise chaining, where you add a call to a new promise in the .then() method of the previous promise you ran. Since .then() is only run after the promise completes execution (succeeds or fails), chaining allows you to run promises sequentially. This stack overflow discussion on the concurrency of Promise.all() arguments suggests that Promise.all() does indeed run the promises that are passed to it asynchronously/simultaneously. This makes sense, since Filer functions are passed as arguments to Promise.all(), the Filer functions all launch their promises when Promise.all() is called. What all this amounted to was the fact that Promises.all() could only be used for the fs.promises.stat() calls because they queried different file names, but the relied on the operations of creating and renaming the file to have been run previously.
The magic of writing asynchronous code is its efficiency, however if you aren't paying attention to how the async functionality is scheduled, you may produce code that consistently runs in the order you want it to whenever you test it, but may choose to behave differently at any time because one promise returned before another (like checking for a file being renamed before it was actually renamed). Such things may sneak right past TravisCI tests, or whatever automated checks you have in place, and make it into a repo only to go haywire later.

Attempt 2: cleaner code with chained promises:

Reviewing Another Contributor's Pull Request

Another task was to review another classmate's pull request

My classmate had chosen something more ambitious than writing a test since they had noticed that Filer did not have a version of Node's mkdtemp() function. Since the function was supposed to mimic Node's mkdtemp() the first thing to do was check Node's documentation for that function.

mkdtemp() has an optional [options] argument. Unfortunately, the Node.js documentation only vaguely mentions that the options argument is used to pass encoding information. More confusingly, it seems as though Node's standard mkdir method does not take an options argument to set encoding. A google search and a look through this archived Node.js issue suggests that the encoding is for the file name's character set. Paths with characters that are not valid in UTF-8 encoding seemed to cause problems in Node.js's fs module, and this archived issue may be an ongoing issue since it continues to be referenced by dev teams on Github.

This led to a discussion on how mkdir() and mkdtemp() worked in the Filer code base. The author of the pull request claimed they ignored the options flag except to include it as an argument in the function definition. They did this because an internal function, make_directory() that was called by both mkdir() and the newly implemented mkdtemp() had no options argument. Changing the function definition to include it would require refactoring other code that referenced it.
Some more research and poking about in the Filer repo revealed that the 'mode' argument for mkdir() wasn't really used in make_directory() either--it was only validated as a proper mode value, and converted to a format to be used elsewhere (in this case, nowhere). However it seemed to be a deliberate decision not to use 'mode' because was used in Node to set directory permissions, and only in Unix-style filesystems (no Windows). Since Filer's Readme.md states that it does not deal with file permissions, it's unclear what we would be doing with 'mode' after we take it in as an argument to mkdir(), or even how you would simulate file permissions in an emulated filesystem that has a single user -- the person currently using the browser.

A follow up discussion with one of the repo owners (humphd) there was apparently a case where file permissions were needed in front end Javascript. He had needed to give simulated file objects to pass to a Unix VM that was running in a front end environment I guess this just shows that if you're going to mimic the functionality of a file system, some creative person will eventually find the need for all of the features that were present in the original.
After checking out the code and documentation, I also made an effort to test the new mkdtemp function, although being unfamiliar with debugging Node.js libraries in VSCode, I opted to write a quick Mocha test to check that mkdtmp() created something (although I would have preferred to set some breakpoints and inspect variables in memory):

This was supposed to create a temporary file, and pass the name of the created file to stat() to check if it existed. This was adapted from the existing test for mkdir(), and passed when it was run.