Merge pull request forcedotcom#87 from maliroteh-sf/main

Fix some unit tests

.mocharc.json

@@ -1,8 +1,7 @@
 {
-  "require": ["ts-node/register"],
-  "watch-extensions": "ts",
-  "recursive": true,
-  "reporter": "spec",
-  "timeout": 600000,
-  "node-option": ["loader=ts-node/esm"]
+    "require": ["ts-node/register"],
+    "watch-extensions": "ts",
+    "recursive": true,
+    "reporter": "spec",
+    "node-option": ["loader=ts-node/esm"]
 }

src/common/CommonUtils.ts

@@ -270,10 +270,7 @@ export class CommonUtils {
 
             CommonUtils.logger.debug(`Executing command: '${fullCommand}'`);
 
-            const prc = childProcess.spawn(command, args, {
-                shell: true,
-                stdio: stdioOptions
-            });
+            const prc = CommonUtils.spawnWrapper(command, args, stdioOptions);
 
             prc.stdout?.on('data', (data) => {
                 capturedStdout.push(data.toString());
@@ -305,6 +302,25 @@ export class CommonUtils {
         });
     }
 
+    /**
+     * Used as a wrapper to child_process spawn function solely for unit testing purposes
+     *
+     * @param command the command to be forwarded to child_process spawn function
+     * @param args the arguments to be forwarded to child_process spawn function
+     * @param stdioOptions  the options to be forwarded to child_process spawn function
+     * @returns the process that is returned as the result of a call to child_process spawn function
+     */
+    public static spawnWrapper(
+        command: string,
+        args: string[] = [],
+        stdioOptions: StdioOptions = ['ignore', 'pipe', 'ignore']
+    ): childProcess.ChildProcess {
+        return childProcess.spawn(command, args, {
+            shell: true,
+            stdio: stdioOptions
+        });
+    }
+
     /**
      * Launches the desktop browser and navigates to the provided URL.
      *

test/unit/.eslintrc.cjs

@@ -3,6 +3,7 @@ module.exports = {
     // Allow describe and it
     env: { mocha: true },
     rules: {
+        'class-methods-use-this': 'off',
         'no-unused-expressions': 'off', // Allow assert style expressions. i.e. expect(true).to.be.true
         '@typescript-eslint/explicit-function-return-type': 'off', // Return types are defined by the source code. Allows for quick overwrites.
         '@typescript-eslint/no-empty-function': 'off', // Mocked out the methods that shouldn't do anything in the tests.

test/unit/common/CommonUtils.test.ts

@@ -6,19 +6,70 @@
  */
 import os from 'node:os';
 import fs from 'node:fs';
+import http from 'node:http';
+import https from 'node:https';
 import path from 'node:path';
-import { Messages } from '@salesforce/core';
+import stream from 'node:stream';
+import * as childProcess from 'node:child_process';
+import { Logger, Messages } from '@salesforce/core';
 import { TestContext } from '@salesforce/core/testSetup';
 import { stubMethod } from '@salesforce/ts-sinon';
 import { expect } from 'chai';
+import sinon from 'sinon';
 import { CommonUtils } from '../../../src/common/CommonUtils.js';
 
 Messages.importMessagesDirectoryFromMetaUrl(import.meta.url);
 
+// Custom Writable stream that writes to memory, for testing downloadFile
+const data: Buffer[] = [];
+class MemoryWriteStream extends stream.Writable {
+    public getData(): string {
+        return Buffer.concat(data).toString();
+    }
+
+    // eslint-disable-next-line @typescript-eslint/no-unused-vars
+    public _write(chunk: Buffer, encoding: string, callback: (error?: Error | null) => void): void {
+        data.push(chunk);
+        callback();
+    }
+
+    public close(): void {
+        // no-op
+    }
+}
+
 describe('CommonUtils', () => {
+    const downloadDestinationFile = path.join(os.tmpdir(), 'ca.crt');
     const messages = Messages.loadMessages('@salesforce/lwc-dev-mobile-core', 'common');
     const $$ = new TestContext();
 
+    let unlinkStub: sinon.SinonStub<any[], any>;
+    let httpGetStub: sinon.SinonStub<any[], any>;
+    let httpsGetStub: sinon.SinonStub<any[], any>;
+    let mockBadResponse: http.IncomingMessage;
+    let mockGoodResponse: http.IncomingMessage;
+    let writeStreamMock: MemoryWriteStream;
+
+    beforeEach(() => {
+        stubMethod($$.SANDBOX, fs, 'createWriteStream').returns(writeStreamMock);
+        unlinkStub = stubMethod($$.SANDBOX, fs, 'unlink').callsFake((_, callback) => {
+            callback();
+        });
+        httpGetStub = stubMethod($$.SANDBOX, http, 'get');
+        httpsGetStub = stubMethod($$.SANDBOX, https, 'get');
+
+        mockBadResponse = new stream.PassThrough() as unknown as http.IncomingMessage;
+        mockBadResponse.statusCode = 404;
+        mockBadResponse.statusMessage = 'Not Found';
+
+        mockGoodResponse = new stream.PassThrough() as unknown as http.IncomingMessage;
+        mockGoodResponse.statusCode = 200;
+        mockGoodResponse.statusMessage = 'Done';
+        (mockGoodResponse as unknown as stream.PassThrough).write('This is a test');
+
+        writeStreamMock = new MemoryWriteStream();
+    });
+
     afterEach(() => {
         $$.restore();
     });
@@ -119,49 +170,46 @@ describe('CommonUtils', () => {
         }
     });
 
-    it('downloadFile function', async () => {
-        const dest = path.join(os.tmpdir(), 'ca.crt');
-
-        // should fail and not create a destination file
-        try {
-            await CommonUtils.downloadFile('badurl', dest);
-        } catch (error) {
-            expect(error).to.be.an('error');
-            expect(fs.existsSync(dest)).to.be.false;
-        }
+    it('downloadFile fails with bad relative url', async () => {
+        await verifyDownloadFails('badurl1', mockBadResponse);
+    });
 
-        // should fail and not create a destination file
-        try {
-            await CommonUtils.downloadFile('http://badurl', dest);
-        } catch (error) {
-            expect(error).to.be.an('error');
-            expect(fs.existsSync(dest)).to.be.false;
-        }
+    it('downloadFile fails with bad http absolute url', async () => {
+        await verifyDownloadFails('http://badurl2', mockBadResponse);
+    });
 
-        // should fail and not create a destination file
-        try {
-            await CommonUtils.downloadFile('https://badurl', dest);
-        } catch (error) {
-            expect(error).to.be.an('error');
-            expect(fs.existsSync(dest)).to.be.false;
-        }
+    it('downloadFile fails with bad https absolute url', async () => {
+        await verifyDownloadFails('https://badurl3', mockBadResponse);
+    });
 
-        // should fail and not create a destination file
-        try {
-            await CommonUtils.downloadFile('https://www.google.com/badurl', dest);
-        } catch (error) {
-            expect(error).to.be.an('error');
-            expect(fs.existsSync(dest)).to.be.false;
-        }
+    it('downloadFile fails when fetching url goes through but saving to file fails', async () => {
+        await verifyDownloadFails('https://www.salesforce.com', {
+            statusCode: 200,
+            pipe: (writeStream: fs.WriteStream) => {
+                writeStream.emit('error', new Error('failed to write to file'));
+            }
+        });
+    });
 
-        // For now don't run this part on Windows b/c our CI
-        // environment does not give file write permission.
-        if (process.platform !== 'win32') {
-            // should pass and create a destination file
-            await CommonUtils.downloadFile('https://www.google.com', dest);
-            expect(fs.existsSync(dest)).to.be.true;
-        }
-    }).timeout(20000); // increase timeout for this test
+    it('downloadFile passes and save the content', async () => {
+        httpsGetStub.callsFake((_, callback) => {
+            setTimeout(
+                () =>
+                    callback({
+                        statusCode: 200,
+                        pipe: (writeStream: fs.WriteStream) => {
+                            writeStream.write('Test Content');
+                            writeStream.emit('finish');
+                        }
+                    }),
+                100
+            );
+            return { on: () => {}, end: () => {} };
+        });
+        await CommonUtils.downloadFile('https://www.salesforce.com', downloadDestinationFile);
+        expect(unlinkStub.called).to.be.false;
+        expect(writeStreamMock.getData()).to.equal('Test Content');
+    });
 
     it('read/write text file functions', async () => {
         const dest = path.join(os.tmpdir(), 'test_file.txt');
@@ -232,37 +280,50 @@ describe('CommonUtils', () => {
         ]);
     });
 
-    // Disabling this test for now. It runs & passes locally but it fails
-    // in CI b/c the child process errors out with `read ENOTCONN` error.
-    /* it('spawnCommandAsync', async () => {
-        const fakeProcess = new ChildProcess();
-        fakeProcess.stdout = process.stdout;
-        fakeProcess.stderr = process.stderr;
-
-        const mockSpawn = jest.fn((): ChildProcess => fakeProcess);
-        jest.spyOn(cp, 'spawn').mockImplementation(mockSpawn);
+    it('spawnCommandAsync handles when child process completes successfully', async () => {
+        const fakeProcess = new childProcess.ChildProcess();
+        const spawnStub = stubMethod($$.SANDBOX, CommonUtils, 'spawnWrapper').returns(fakeProcess);
+        const errorLoggerMock = stubMethod($$.SANDBOX, Logger.prototype, 'error');
 
         setTimeout(() => {
-            fakeProcess.stdout?.push('Test STDOUT');
-            fakeProcess.stderr?.push('Test STDERR');
             fakeProcess.emit('close', 0);
-            fakeProcess.kill(0);
-        }, 2000);
+        }, 100);
 
-        const results = await CommonUtils.spawnCommandAsync(
-            'cmd',
+        await CommonUtils.spawnCommandAsync('somecommand', ['arg1', 'arg2'], ['ignore', 'inherit', 'pipe']);
+
+        expect(spawnStub.calledOnce);
+        expect(spawnStub.getCall(0).args).to.deep.equal([
+            'somecommand',
             ['arg1', 'arg2'],
             ['ignore', 'inherit', 'pipe']
-        );
+        ]);
+        expect(errorLoggerMock.called).to.be.false;
+    });
 
-        expect(mockSpawn).toHaveBeenCalledWith('cmd', ['arg1', 'arg2'], {
-            shell: true,
-            stdio: ['ignore', 'inherit', 'pipe']
-        });
+    it('spawnCommandAsync handles when child process completes with error', async () => {
+        const fakeProcess = new childProcess.ChildProcess();
+        const spawnStub = stubMethod($$.SANDBOX, CommonUtils, 'spawnWrapper').returns(fakeProcess);
+        const errorLoggerMock = stubMethod($$.SANDBOX, Logger.prototype, 'error');
 
-        expect(results.stdout).toBe('Test STDOUT');
-        expect(results.stderr).toBe('Test STDERR');
-    });*/
+        setTimeout(() => {
+            fakeProcess.emit('close', 123);
+        }, 100);
+
+        try {
+            await CommonUtils.spawnCommandAsync('somecommand', ['arg1', 'arg2'], ['ignore', 'inherit', 'pipe']);
+        } catch (error) {
+            expect(error).to.be.an('error');
+        }
+
+        expect(spawnStub.calledOnce);
+        expect(spawnStub.getCall(0).args).to.deep.equal([
+            'somecommand',
+            ['arg1', 'arg2'],
+            ['ignore', 'inherit', 'pipe']
+        ]);
+        expect(errorLoggerMock.called).to.be.true;
+        expect(errorLoggerMock.getCall(0).args[0]).to.contain('somecommand arg1 arg2');
+    });
 
     function createStats(isFile: boolean): fs.Stats {
         return {
@@ -300,4 +361,25 @@ describe('CommonUtils', () => {
             parentPath: fname
         };
     }
+
+    async function verifyDownloadFails(url: string, mockResponse: any) {
+        const fake = (options: any, callback: (res: http.IncomingMessage) => void) => {
+            setTimeout(() => callback(mockResponse), 100);
+            return { on: () => {}, end: () => {} };
+        };
+
+        if (url.startsWith('https:')) {
+            httpsGetStub.callsFake(fake);
+        } else {
+            httpGetStub.callsFake(fake);
+        }
+
+        try {
+            await CommonUtils.downloadFile(url, downloadDestinationFile);
+        } catch (error) {
+            expect(error).to.be.an('error');
+            expect(unlinkStub.calledOnce).to.be.true;
+            expect(unlinkStub.getCall(0).args[0]).to.be.equal(downloadDestinationFile);
+        }
+    }
 });