refactor(data_dir): simplify logic and make code robust and testable

[pano9000]

Hi,

this PR aims to refactor the data_dir.ts file.

• make code more DRY, reducing duplicate code
• switch to using path.join instead of manual path concatenation using path.sep
• reordered logic in the getTriliumDataDir function to be more easy to follow the order of "trilium data path order of priority"
• added tests for these functions (except for getTriliumDataDir -> see below please)
• replacing previously exported values with a proper readonly object, that is now also testable

regarding the getTriliumDataDir tests:
since this function is also using fs related methods (i.e. checking for folder existance and folder creation), I have to ways on how to make the function testable:

a) using a simple DI object, to "inject" these dependencies (fs methods, etc.) which will allow to mock these functions in the tests, e.g. like so:

export function getTriliumDataDir(di = {createDirIfNotExisting, existsSync: fs.existsSync, getPlatformAppDataDir}) {
...
di.createDirIfNotExisting(...)
...
}

in the tests, these would be swapped out by mocks, in the "production" code these are just using the real methods by default.

b) using the real methods and then having to make sure folders are cleaned up afterwards

I personally would prefer a) however I did not see this technique being used in the repo so far, so would like to have an OK before I go that route?

@eliandoran what do you think?

[eliandoran]

@pano9000 , I think method A is a bit verbose for the JavaScript world.
Method B, although closer to reality can result in test flakiness since you also have to deal with the clean-up.
My recommendation is method C: keep using FS as is, and mock the API calls in the tests: https://jestjs.io/docs/manual-mocks#mocking-node-modules

[pano9000]

thanks for the reply, I don't like method B) either to be honest, so method C) would be a good middle end, but it might require additional dev Dependencies.

Currently I can see jasmine as devDependency already, but I have no experience with it and running the npm run test-jasmine command also is not too helpful currently – do you know if this is used somewhere already?

I am almost inclined to do method A) locally (without commiting) to run all tests and gather confidence that everything works as expected, and then push out this test into a separate PR – depending on which test runner framework the decision falls on.

(with some manual tests I did locally everything seems to have been working properly already).

[pano9000]

I've pushed aside the DI idea above and am working on your suggested method C) straight away now – I am doing it with the built-in node:test features for now locally – once the test logic / cases are defined, we can easily adapt them to whatever test runner framework

[pano9000]

@eliandoran I've written a full test that confirms all different cases are working as expected.
I've used the built-in mock functions of node:test, which addmittedly currently are available only via --experimental-test-module-mocks.

so not sure, if I should even commit that test or not?

you can run it, using the following (of course adjust the path accordingly):

node  --experimental-transform-types --experimental-test-module-mocks --experimental-test-coverage spec-es6/data_dir_node.spec.ts

import assert from "node:assert";
import { mock, describe, it, before, beforeEach } from "node:test";

import type { getTriliumDataDir as getTriliumDataDirType } from "../src/services/data_dir";


describe("#getTriliumDataDir", () => {
  let getTriliumDataDir: typeof getTriliumDataDirType;

  const mockFn = {
    "existsSyncMock": mock.fn(),
    "mkdirSyncMock": mock.fn(),
    "osHomedirMock": mock.fn(),
    "osPlatformMock": mock.fn(),
    "pathJoinMock": mock.fn(),
  };

  const fsMock = mock.module("node:fs", {
    namedExports: {
      existsSync: mockFn.existsSyncMock,
      mkdirSync: mockFn.mkdirSyncMock
    }
  })

  const osMock = mock.module("node:os", {
    namedExports: {
      homedir: mockFn.osHomedirMock,
      platform: mockFn.osPlatformMock
    }
  });

  const pathMock = mock.module("path", {
    namedExports: {
      join: mockFn.pathJoinMock
    }
  });

  // helper to reset call counts
  const resetAllMockCallCounts = () => {
    Object.values(mockFn).forEach(mockedFn => {
      mockedFn.mock.resetCalls()
    })
  }

  const setMockPlatform = (osPlatform: string, homedir: string, pathJoin: string) => {
    // @ts-expect-error - types for mockImplementation seem to be wrongly expecting undefined as return value
    mockFn.osPlatformMock.mock.mockImplementation(() => osPlatform);
    // @ts-expect-error
    mockFn.osHomedirMock.mock.mockImplementation(() => homedir);
    // @ts-expect-error
    mockFn.pathJoinMock.mock.mockImplementation(() => pathJoin );
  };

  before(async () => {
    ({ getTriliumDataDir } = await import('../src/services/data_dir.ts'));
  })

  beforeEach(async () => {
    // make sure these are not set
    delete process.env.TRILIUM_DATA_DIR;
    delete process.env.APPDATA;
    resetAllMockCallCounts();
  });

  describe("case A – process.env.TRILIUM_DATA_DIR is set", () => {

    it("when folder exists – it should return the path, without attempting to create the folder", async () => {

      const mockTriliumDataPath = "/home/mock/trilium-data-ENV-A1"
      process.env.TRILIUM_DATA_DIR = mockTriliumDataPath;

      // set fs.existsSync to true, i.e. the folder does exist
      // @ts-expect-error - types for mockImplementation seem to be wrongly expecting undefined as return value
      mockFn.existsSyncMock.mock.mockImplementation(() => true);

      const result = getTriliumDataDir("trilium-data");

      // createDirIfNotExisting should call existsync 1 time and mkdirSync 0 times -> as it does not need to create the folder
      // and return value should be TRILIUM_DATA_DIR value from process.env
      assert.equal(mockFn.existsSyncMock.mock.callCount(), 1);
      assert.equal(mockFn.mkdirSyncMock.mock.callCount(), 0);
      assert.strictEqual(result, process.env.TRILIUM_DATA_DIR);
    })

    it("when folder does not exist – it should attempt to create the folder and return the path", async () => {
      const mockTriliumDataPath = "/home/mock/trilium-data-ENV-A2"
      process.env.TRILIUM_DATA_DIR = mockTriliumDataPath;

      // set fs.existsSync mock to return false, i.e. the folder does not exist
      // @ts-expect-error - types for mockImplementation seem to be wrongly expecting undefined as return value
      mockFn.existsSyncMock.mock.mockImplementation(() => false);

      const result = getTriliumDataDir("trilium-data");

      // createDirIfNotExisting should call existsync 1 time and mkdirSync 1 times -> as it has to create the folder
      // and return value should be TRILIUM_DATA_DIR value from process.env
      assert.equal(mockFn.existsSyncMock.mock.callCount(), 1);
      assert.equal(mockFn.mkdirSyncMock.mock.callCount(), 1);
      assert.strictEqual(result, mockTriliumDataPath);

    })

  })

  describe("case B – process.env.TRILIUM_DATA_DIR is not set and Trilium folder is existing in platform's home dir", () => {

    it("it should check if folder exists and return it", async () => {

      const homedir = "/home/mock";
      const dataDirName = "trilium-data";
      const mockTriliumDataPath = `${homedir}/${dataDirName}`

      // @ts-expect-error - types for mockImplementation seem to be wrongly expecting undefined as return value
      mockFn.pathJoinMock.mock.mockImplementation(() => mockTriliumDataPath );

      // set fs.existsSync to true, i.e. the folder does exist
      // @ts-expect-error - types for mockImplementation seem to be wrongly expecting undefined as return value
      mockFn.existsSyncMock.mock.mockImplementation(() => true);

      const result = getTriliumDataDir(dataDirName);

      assert.strictEqual(result, mockTriliumDataPath)
      assert.equal(mockFn.existsSyncMock.mock.callCount(), 1);

    })

  })


  describe("case C – process.env.TRILIUM_DATA_DIR is not set and Trilium folder is not existing in platform's home dir", () => {

    it("w/ Platform 'Linux', an existing App Data Folder (~/.local/share) but non-existing Trilium dir (~/.local/share/trilium-data) – it should attempt to create the dir", async () => {

      const homedir = "/home/mock";
      const dataDirName = "trilium-data";
      const mockPlatformDataPath = `${homedir}/.local/share/${dataDirName}`

      // mock set: os.platform, os.homedir and pathJoin return values
      setMockPlatform("linux", homedir, mockPlatformDataPath);

      // use Generator to precisely control order of fs.existSync return values
      const existsSyncMockGen = (function* () {
        // 1) fs.existSync -> case B
        yield false;
        // 2) fs.existSync -> case C -> checking if default OS PlatformAppDataDir exists
        yield true;
        // 3) fs.existSync -> case C -> checking if Trilium Data folder exists
        yield false;
      })()

      //@ts-expect-error
      mockFn.existsSyncMock.mock.mockImplementation(() => existsSyncMockGen.next().value);

      const result = getTriliumDataDir(dataDirName);

      assert.equal(mockFn.existsSyncMock.mock.callCount(), 3);
      assert.equal(mockFn.mkdirSyncMock.mock.callCount(), 1);
      assert.strictEqual(result, mockPlatformDataPath);

    })

    it("w/ Platform Linux, an existing App Data Folder (~/.local/share) AND an existing Trilium Data dir – it should return path to the dir", async () => {

      const homedir = "/home/mock";
      const dataDirName = "trilium-data";
      const mockPlatformDataPath = `${homedir}/.local/share/${dataDirName}`

      // mock set: os.platform, os.homedir and pathJoin return values
      setMockPlatform("linux", homedir, mockPlatformDataPath);

      // use Generator to precisely control order of fs.existSync return values
      const existsSyncMockGen = (function* () {
        // 1) fs.existSync -> case B
        yield false;
        // 2) fs.existSync -> case C -> checking if default OS PlatformAppDataDir exists
        yield true;
        // 3) fs.existSync -> case C -> checking if Trilium Data folder exists
        yield true;
      })();

      //@ts-expect-error - types for mockImplementation seem to be wrongly expecting undefined as return value
      mockFn.existsSyncMock.mock.mockImplementation(() => existsSyncMockGen.next().value);

      const result = getTriliumDataDir(dataDirName);

      assert.strictEqual(result, mockPlatformDataPath);
      assert.equal(mockFn.existsSyncMock.mock.callCount(), 3);
      assert.equal(mockFn.mkdirSyncMock.mock.callCount(), 0);
    })

    it("w/ Platform 'win32' and set process.env.APPDATA behaviour", async () => {

      const homedir = "C:\\Users\\mock";
      const dataDirName = "trilium-data";
      const appDataDir = `${homedir}\\AppData\\Roaming`;
      const mockPlatformDataPath = `${appDataDir}\\${dataDirName}`;
      process.env.APPDATA = `${appDataDir}`;

      // mock set: os.platform, os.homedir and pathJoin return values
      setMockPlatform("win32", homedir, mockPlatformDataPath);

      // use Generator to precisely control order of fs.existSync return values
      const existsSyncMockGen = (function* () {
        // 1) fs.existSync -> case B
        yield false;
        // 2) fs.existSync -> case C -> checking if default OS PlatformAppDataDir exists
        yield true;
        // 3) fs.existSync -> case C -> checking if Trilium Data folder exists
        yield false;
      })();

      //@ts-expect-error - types for mockImplementation seem to be wrongly expecting undefined as return value
      mockFn.existsSyncMock.mock.mockImplementation(() => existsSyncMockGen.next().value);

      const result = getTriliumDataDir(dataDirName);

      assert.strictEqual(result, mockPlatformDataPath);
      assert.equal(mockFn.existsSyncMock.mock.callCount(), 3);
      assert.equal(mockFn.mkdirSyncMock.mock.callCount(), 1);
    })

  })

  describe("case D – fallback to creating Trilium folder in home dir", () => {

    it("w/ unknown PlatformAppDataDir it should attempt to create the folder in the homefolder", async () => {

      const homedir = "/home/mock";
      const dataDirName = "trilium-data";
      const mockPlatformDataPath = `${homedir}/${dataDirName}`;

      setMockPlatform("aix", homedir, mockPlatformDataPath);

      const existsSyncMockGen = (function* () {
        // first fs.existSync -> case B -> checking if folder exists in home folder
        yield false;
        // second fs.existSync -> case D -> triggered by createDirIfNotExisting
        yield false;
      })();

      //@ts-expect-error - types for mockImplementation seem to be wrongly expecting undefined as return value
      mockFn.existsSyncMock.mock.mockImplementation(() => existsSyncMockGen.next().value);

      const result = getTriliumDataDir(dataDirName);

      assert.equal(mockFn.existsSyncMock.mock.callCount(), 2);
      assert.equal(mockFn.mkdirSyncMock.mock.callCount(), 1);
      assert.strictEqual(result, `${mockPlatformDataPath}`);
    })

  })
})