test: migrate to `jest`, leverage asar header and unpacked dir snapshots, and get swoll 💪 #355

mmaietta · 2025-02-08T17:37:21Z

This is a test-only upgrade PR
PR diff line changes may seem large, but they're largely devDeps yarn.lock (jest, ts-jest, etc @ 2.3k lines) and snapshot files (~1.3k lines). This still runs unit tests in both node and electron/main test environments leveraging jest's testRunner functionality 🙂

Looking to achieve a few things with this PR

Prep work / adding robustness for bug fix PRs and additional coverage for node22 upgrade
Migrate to jest, which will make an even easier transition to vitest post-node22 upgrade (plug-n-play swap IIRC)
- Also dramatically speeds up test suite execution duration
Migrates test files to typescript 🎉
Additional snapshot coverage (instead of fixtures) for capturing additional details (described below)

Functionally, this PR adds:

GHA test.yaml -> fail-fast: false allows the matrix jobs to continue even when one fails. This will allow verifying asar behavior across each node version before the overall test is marked as a failure.
verifySmartUnpack helper function that does the following:
- verifyHeader - parses out unstable properties ("offset" varies per build machine?), converts all paths (e.g. link) to system independent strings, and then stores it as a snapshot
- verifyFileTree - Traverses a directory and snapshots the tree hierarchy and plain text files (also used for unpacked dir snapshots)
createSymlinkedApp - Creates a new app at runtime from the app template, adds folders, symlinks, and allows you to pass in additionalFiles that you can use to customize each app's directory
Test helper functions
- it.ifWindows = (os.platform() === 'win32') ? test : test.skip to be used instead of embedding tests within an if statement (the latter causes snapshots to be deleted on different OS's, but using a test.skip retains a snapshot)
- A corresponding it.ifNotWindows
- Usage: it.ifNotWindows("some test trying to extra a symlink from an asar", () => { ... })
Fixes fixture comparison with await as previously it was doing a strict equals on two Promises
Enables a migration to docker containers for running jest so that we can run linux-target unit tests locally on an arm64 mac or windows machine. This should streamline local testing environments and also consolidate tests into an ephemeral test environment (will be a follow-up PR)

With verifySmartUnpack, the header snapshot allows finer granularity of what is happening under the hood, especially with regards to symlinks and integrity calculations. Example respective snapshots excerpts below:

    "var": {
      "link": "private/var",
      "unpacked": true,
    },

"dir1": {
 "files": {
   "file1.txt": { 
     "integrity": {
       "algorithm": "SHA256",
       "blockSize": 4194304,
       "blocks": [
         "420149d3f852894ba7f32e9d3ec7d8919ee2451724bf2580b0186cd373bd6d82",
       ],
       "hash": "420149d3f852894ba7f32e9d3ec7d8919ee2451724bf2580b0186cd373bd6d82",
     },
     "size": 9,
     "unpacked": true,
   },
 },
}

Then the "walk" snapshot functionality allows all these lines to become just included in verifySmartUnpack('tmp/packthis-unpack-subdir-cli.asar')

asar/test/cli-spec.js

Lines 182 to 185 in 121efeb

    
           assert.ok(fs.existsSync('tmp/packthis-unpack-subdir-cli.asar.unpacked/file0.txt')); 
        
           assert.ok(fs.existsSync('tmp/packthis-unpack-subdir-cli.asar.unpacked/dir1/file1.txt')); 
        
           assert.ok(fs.existsSync('tmp/packthis-unpack-subdir-cli.asar.unpacked/dir2/subdir/file2.png')); 
        
           assert.ok(fs.existsSync('tmp/packthis-unpack-subdir-cli.asar.unpacked/dir2/subdir/file3.txt'));

…re's variance between node versions)

…ain snapshots when running on different OS's

felixrieseberg · 2025-02-10T20:55:00Z

test/setup/jest.env.setup.ts

+import { platform } from 'os';
+
+test.if = (condition: boolean) => (condition ? test : test.skip);
+test.ifWindows = test.if(platform() === 'win32');


Very neat, thanks!

felixrieseberg

Looks generally good! Left a few tiny stylistic things but I think we can ✅ this PR once merge conflicts are fixed

felixrieseberg · 2025-02-10T20:58:35Z

test/setup/jest.global.setup.ts

+import fs from '../../lib/wrapped-fs';
+
+export default () => {
+  rimraf.sync(path.join(__dirname, '..', '..', 'tmp'), fs);


This looks a little scary on first read and could maybe use a little comment

Haha yeah, fair point. It was copied from the mocha setup. Will add a comment

Converted it to a constant in new file ./util/constants with comment on it as to what ROOT_DIR and TEST_APPS_DIR are being used for

felixrieseberg · 2025-02-10T20:59:10Z

test/util/transformStream.ts

  constructor() {
    super();
    this._data = '';
  }

-  _transform(buf, enc, cb) {
+  _transform(buf: any, enc: any, cb: () => any) {


Should we use unknown here?

Either way works for me. tsc was just throwing an error when it was converted to .ts. I'll adjust to unknown or find a proper type for them

Updated.

Note: new definition is _transform(chunk: any, _encoding: string, cb: TransformCallback) that directly copies the API from @types/node/stream.d.ts

felixrieseberg · 2025-02-10T21:00:11Z

test/util/verifySmartUnpack.ts

+
+const rootDir = path.resolve(__dirname, '..', '..');
+
+export const verifySmartUnpack = async (asarPath: string) => {


I don't have a strong opinion, I'm just curious - why export const declarations for these functions but export function for the ones below?

Yeahhh, I'll tidy that up! Had copied some helper code from electron-builder and hadn't modified those. I'll fix it

Updated all test functions to be export const. I didn't modify src code even though those are export function. I personally prefer arrow functions, but am happy to adjust if consistency across src+test is preferred.

…-tests # Conflicts: # test/cli-spec.ts # test/util/createSymlinkApp.js

dsanders11

I think this needs further consideration before landing, so throwing a "request changes" on just to ensure it doesn't merge before that happens.

The current tests use electron-mocha to also run the tests inside Electron's main process. With these changes that's no longer true. That's not necessarily a bad thing, but one we consider properly before landing (and would also remove more dependencies).

Probably a good topic to discuss in the next WG meeting, to gather more historical context on why that choice as made originally.

mmaietta · 2025-02-10T23:00:08Z

@dsanders11 fantastic callout. I plan to be at the upcoming WG meeting as well.

Re: electron-mocha, I believe that jest emulates that simply with testEnvironment: 'node' (as opposed to jsdom being the renderer process). That was my understanding from a small article: https://dev.to/woovi/electron-testing-best-practices-testing-main-and-renderer-code-with-jest-4b5m

dsanders11 · 2025-02-10T23:06:59Z

Re: electron-mocha, I believe that jest emulates that simply with testEnvironment: 'node' (as opposed to jsdom being the renderer process). That was my understanding from a small article: https://dev.to/woovi/electron-testing-best-practices-testing-main-and-renderer-code-with-jest-4b5m

That ensures that it's testing in a Node-like environment (aka no DOM), but the original testing setup in this repo ran the tests actually inside an Electron main process, which would catch issues that jest won't.

… and `electron/main` processes w/ `xvfb-maybe`. Removed `electron-mocha` from devDeps

…-tests # Conflicts: # test/cli-spec.ts # test/util/createSymlinkApp.js

mmaietta · 2025-02-11T02:29:33Z

Thank you @dsanders11 for clarifying! I've updated PR to leverage jest's test runner property to enable running the tests in both node and electron/main environments.
Uses https://github.com/kayahr/jest-electron-runner under-the-hood, which is a fork of https://github.com/facebook-atom/jest-electron-runner but updated to support jest v27+. I double checked the source code to make sure nothing seemed amiss/adverse, it LGTM. Not sure what the electron governance policy is around adding dev deps though

mmaietta · 2025-02-12T16:03:00Z

Hey folks, I've proposed a different approach that still leverages mocha+electron-mocha to reduce changes introduced and not migrate to jest. It still is able to provide the same snapshot functionality (asar header + asar unpacked dir) using a mocha-chai plugin (hope that's okay)
#357
Curious on your thoughts of it as an alternative approach?

mmaietta added 7 commits February 8, 2025 07:50

tmpsave

f3fa966

adding types and all tests pass!

c790814

expanding functionality of verifying

f02ac49

converting remaining files require->import

76342f4

migrating to use verifySmartUnpack and verifyFileTree

ba6a498

disable fail-fast so that all test runners fully execute (in case the…

dcb8387

…re's variance between node versions)

add helper test.if, ifWindows, and ifNotWindows function to ret…

289004b

…ain snapshots when running on different OS's

mmaietta marked this pull request as ready for review February 10, 2025 18:04

mmaietta requested a review from a team as a code owner February 10, 2025 18:04

felixrieseberg reviewed Feb 10, 2025

View reviewed changes

Merge commit 'b2f73c8e5ee9ef64106513c55e67f6fd5d1f576f' into snapshot…

ea7e17e

…-tests # Conflicts: # test/cli-spec.ts # test/util/createSymlinkApp.js

felixrieseberg approved these changes Feb 10, 2025

View reviewed changes

pr feedback :)

1a0728f

dsanders11 requested changes Feb 10, 2025

View reviewed changes

mmaietta added 2 commits February 10, 2025 17:57

add jest-electron-runner to jest config to run tests in both node…

39a5b51

… and `electron/main` processes w/ `xvfb-maybe`. Removed `electron-mocha` from devDeps

Merge commit '9b7ccfec05292d47ea82cb44cd45339bc058febc' into snapshot…

9aa6a00

…-tests # Conflicts: # test/cli-spec.ts # test/util/createSymlinkApp.js

mmaietta added 2 commits February 10, 2025 20:04

clean up code & tsconfig.jest.json

53c9bbe

clear TEST_APPS_DIR in beforeEach by using --runInBand.

036b2d3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: migrate to `jest`, leverage asar header and unpacked dir snapshots, and get swoll 💪 #355

test: migrate to `jest`, leverage asar header and unpacked dir snapshots, and get swoll 💪 #355

mmaietta commented Feb 8, 2025 •

edited

Loading

felixrieseberg Feb 10, 2025

felixrieseberg left a comment

felixrieseberg Feb 10, 2025

mmaietta Feb 10, 2025

mmaietta Feb 10, 2025

felixrieseberg Feb 10, 2025

mmaietta Feb 10, 2025

mmaietta Feb 10, 2025

felixrieseberg Feb 10, 2025

mmaietta Feb 10, 2025

mmaietta Feb 10, 2025

dsanders11 left a comment

mmaietta commented Feb 10, 2025

dsanders11 commented Feb 10, 2025

mmaietta commented Feb 11, 2025

mmaietta commented Feb 12, 2025

	assert.ok(fs.existsSync('tmp/packthis-unpack-subdir-cli.asar.unpacked/file0.txt'));
	assert.ok(fs.existsSync('tmp/packthis-unpack-subdir-cli.asar.unpacked/dir1/file1.txt'));
	assert.ok(fs.existsSync('tmp/packthis-unpack-subdir-cli.asar.unpacked/dir2/subdir/file2.png'));
	assert.ok(fs.existsSync('tmp/packthis-unpack-subdir-cli.asar.unpacked/dir2/subdir/file3.txt'));


		const rootDir = path.resolve(__dirname, '..', '..');

		export const verifySmartUnpack = async (asarPath: string) => {

test: migrate to jest, leverage asar header and unpacked dir snapshots, and get swoll 💪 #355

Are you sure you want to change the base?

test: migrate to jest, leverage asar header and unpacked dir snapshots, and get swoll 💪 #355

Conversation

mmaietta commented Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

felixrieseberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsanders11 left a comment

Choose a reason for hiding this comment

mmaietta commented Feb 10, 2025

dsanders11 commented Feb 10, 2025

mmaietta commented Feb 11, 2025

mmaietta commented Feb 12, 2025

test: migrate to `jest`, leverage asar header and unpacked dir snapshots, and get swoll 💪 #355

test: migrate to `jest`, leverage asar header and unpacked dir snapshots, and get swoll 💪 #355

mmaietta commented Feb 8, 2025 •

edited

Loading