Yarn Workspaces Follow Up (#481)

* Fix topo sort trying to read from packages that aren't within the repository

* Fix incorrect documentation url references

* Fix coverage also including test utility files

* Fix devserver vite config not being linted

* Fix incorrect building commands for devserver

* Fix the artifact command

* Update workflows to not execute jobs at all if there were no detected changes for the corresponding package

* Update matrix jobs to properly not execute when they don't have to

* Make devserver and docserver still execute even if the tabs or libraries job doesn't execute

* Make end github job execute properly

* Remove trailing spaces

* Test including lockfile information for hasChanges

* Linting and lodash import path fixes

* Fix incorrect package name processing

* Fix broken package names being used in commands

* Add tests for lockfile functions

* Minor documentation fixes

* Add tests to command options

* FIx missing icons

* Fix typo

* Fix lint

* Avoid soft breaks

* Fix lint

Author: leeyi45

.github/actions/package.json

@@ -13,7 +13,8 @@
     "@actions/artifact": "^2.3.2",
     "@actions/core": "^1.11.1",
     "@actions/exec": "^1.1.1",
-    "lodash": "^4.17.21"
+    "lodash": "^4.17.21",
+    "snyk-nodejs-lockfile-parser": "^2.4.2"
   },
   "scripts": {
     "build": "node ./build.js",

.github/actions/src/__tests__/commons.test.ts

@@ -8,7 +8,7 @@ vi.mock(import('lodash/memoize.js'), () => ({
 
 const mockedExecOutput = vi.spyOn(exec, 'getExecOutput');
 
-describe(commons.checkForChanges, () => {
+describe(commons.checkDirForChanges, () => {
   function mockChanges(value: boolean) {
     mockedExecOutput.mockResolvedValueOnce({
       exitCode: value ? 1 : 0, stdout: '', stderr: ''
@@ -17,14 +17,14 @@ describe(commons.checkForChanges, () => {
 
   it('should return true if git diff exits with non zero code', async () => {
     mockChanges(true);
-    await expect(commons.checkForChanges('/')).resolves.toEqual(true);
+    await expect(commons.checkDirForChanges('/')).resolves.toEqual(true);
     expect(mockedExecOutput).toHaveBeenCalledOnce();
   });
 
   it('should return false if git diff exits with 0', async () => {
     mockChanges(false);
 
-    await expect(commons.checkForChanges('/')).resolves.toEqual(false);
+    await expect(commons.checkDirForChanges('/')).resolves.toEqual(false);
     expect(mockedExecOutput).toHaveBeenCalledOnce();
   });
 });

.github/actions/src/commons.ts

@@ -60,19 +60,12 @@ export function isPackageRecord(obj: unknown): obj is PackageRecord {
   return true;
 }
 
-// Not using the repotools version since this uses @action/exec instead of
-// calling execFile from child_process
-export async function getGitRoot() {
-  const { stdout } = await getExecOutput('git rev-parse --show-toplevel');
-  return stdout.trim();
-}
-
 /**
  * Returns `true` if there are changes present in the given directory relative to
  * the master branch\
  * Used to determine, particularly for libraries, if running tests and tsc are necessary
  */
-export const checkForChanges = memoize(async (directory: string) => {
+export const checkDirForChanges = memoize(async (directory: string) => {
   const { exitCode } = await getExecOutput(
     'git',
     ['--no-pager', 'diff', '--quiet', 'origin/master', '--', directory],

.github/actions/src/gitRoot.ts

@@ -0,0 +1,13 @@
+import { getExecOutput } from '@actions/exec';
+
+// Not using the repotools version since this uses @action/exec instead of
+// calling execFile from child_process
+async function getGitRoot() {
+  const { stdout } = await getExecOutput('git rev-parse --show-toplevel');
+  return stdout.trim();
+}
+
+/**
+ * Path to the root of the git repository
+ */
+export const gitRoot = await getGitRoot();

.github/actions/src/info/__tests__/index.test.ts

@@ -1,11 +1,13 @@
 import type { Dirent } from 'fs';
 import fs from 'fs/promises';
 import pathlib from 'path';
+import * as core from '@actions/core';
 import { describe, expect, test, vi } from 'vitest';
 import * as git from '../../commons.js';
-import { getAllPackages, getRawPackages } from '../index.js';
+import * as lockfiles from '../../lockfiles/index.js';
+import { getAllPackages, getRawPackages, main } from '../index.js';
 
-const mockedCheckChanges = vi.spyOn(git, 'checkForChanges');
+const mockedCheckChanges = vi.spyOn(git, 'checkDirForChanges');
 
 vi.mock(import('path'), async importOriginal => {
   const { posix } = await importOriginal();
@@ -16,6 +18,10 @@ vi.mock(import('path'), async importOriginal => {
   };
 });
 
+vi.mock(import('../../gitRoot.js'), () => ({
+  gitRoot: 'root'
+}));
+
 class NodeError extends Error {
   constructor(public readonly code: string) {
     super();
@@ -39,7 +45,7 @@ const mockDirectory: Record<string, string | Record<string, unknown>> = {
         'package.json': JSON.stringify({
           name: '@sourceacademy/bundle-bundle0',
           devDependencies: {
-            '@sourceacademy/modules-lib': 'workspace:^'
+            '@sourceacademy/modules-lib': 'workspace:^',
           }
         })
       },
@@ -49,7 +55,8 @@ const mockDirectory: Record<string, string | Record<string, unknown>> = {
         'package.json': JSON.stringify({
           name: '@sourceacademy/tab-Tab0',
           dependencies: {
-            '@sourceacademy/bundle-bundle0': 'workspace:^'
+            lodash: '^4.1.1',
+            '@sourceacademy/bundle-bundle0': 'workspace:^',
           },
           devDependencies: {
             playwright: '^1.54.0'
@@ -114,6 +121,7 @@ function mockReadFile(path: string) {
 
 vi.spyOn(fs, 'readdir').mockImplementation(mockReaddir as any);
 vi.spyOn(fs, 'readFile').mockImplementation(mockReadFile as any);
+vi.spyOn(lockfiles, 'hasLockFileChanged').mockResolvedValue(false);
 
 describe(getRawPackages, () => {
   test('maxDepth = 1', async () => {
@@ -125,7 +133,7 @@ describe(getRawPackages, () => {
     const [[name, packageData]] = results;
     expect(name).toEqual('@sourceacademy/modules');
     expect(packageData.hasChanges).toEqual(true);
-    expect(git.checkForChanges).toHaveBeenCalledOnce();
+    expect(git.checkDirForChanges).toHaveBeenCalledOnce();
   });
 
   test('maxDepth = 3', async () => {
@@ -211,3 +219,33 @@ describe(getAllPackages, () => {
     expect(results['@sourceacademy/modules-lib'].changes).toEqual(true);
   });
 });
+
+describe(main, () => {
+  const mockedSetOutput = vi.spyOn(core, 'setOutput');
+
+  vi.spyOn(core.summary, 'addHeading').mockImplementation(() => core.summary);
+  vi.spyOn(core.summary, 'addTable').mockImplementation(() => core.summary);
+  vi.spyOn(core.summary, 'write').mockImplementation(() => Promise.resolve(core.summary));
+
+  test('Does not write packages with no changes to the output', async () => {
+    mockedCheckChanges.mockImplementation(path => {
+      return Promise.resolve(path === 'root/src/tabs/tab0');
+    });
+
+    await main();
+    const { mock: { calls } } = mockedSetOutput;
+
+    expect(mockedSetOutput).toHaveBeenCalledTimes(6);
+
+    expect(calls[0]).toEqual(['bundles', []]);
+    expect(calls[1]).toEqual(['tabs', [expect.objectContaining({ changes: true })]]);
+    expect(calls[2]).toEqual(['libs', []]);
+
+    // These next two are undefined because the mock implementations
+    // don't return any info about them
+    expect(calls[3]).toEqual(['devserver', undefined]);
+    expect(calls[4]).toEqual(['docserver', undefined]);
+
+    expect(calls[5]).toEqual(['workflows', false]);
+  });
+});

.github/actions/src/info/action.yml

@@ -3,11 +3,11 @@ description: Gets which packages are currently present in the repository
 
 outputs:
   libs:
-    description: The list of library packages present
+    description: The list of library packages that have changes
   bundles:
-    description: The list of bundles packages present
+    description: The list of bundles packages that have changes
   tabs:
-    description: The list of tabs packages present
+    description: The list of tabs packages that have changes
   devserver:
     description: Information for the devserver
   docserver:

.github/actions/src/info/index.ts

@@ -4,7 +4,9 @@ import utils from 'util';
 import * as core from '@actions/core';
 import type { SummaryTableRow } from '@actions/core/lib/summary.js';
 import packageJson from '../../../../package.json' with { type: 'json' };
-import { checkForChanges, getGitRoot, type PackageRecord, type RawPackageRecord } from '../commons.js';
+import { checkDirForChanges, type PackageRecord, type RawPackageRecord } from '../commons.js';
+import { gitRoot } from '../gitRoot.js';
+import { getPackagesWithResolutionChanges, hasLockFileChanged } from '../lockfiles/index.js';
 import { topoSortPackages } from './sorter.js';
 
 const packageNameRE = /^@sourceacademy\/(.+?)-(.+)$/u;
@@ -14,23 +16,34 @@ const packageNameRE = /^@sourceacademy\/(.+?)-(.+)$/u;
  * an unprocessed format
  */
 export async function getRawPackages(gitRoot: string, maxDepth?: number) {
+  let packagesWithResolutionChanges: Set<string> | null = null;
+
+  // If there are lock file changes we need to set hasChanges to true for
+  // that package even if that package's directory has no changes
+  if (await hasLockFileChanged()) {
+    packagesWithResolutionChanges = await getPackagesWithResolutionChanges();
+  }
+
   const output: Record<string, RawPackageRecord> = {};
 
+  /**
+   * Search the given directory for package.json files
+   */
   async function recurser(currentDir: string, currentDepth: number) {
     const items = await fs.readdir(currentDir, { withFileTypes: true });
     await Promise.all(items.map(async item => {
       if (item.isFile()) {
         if (item.name === 'package.json') {
           try {
             const [hasChanges, packageJson] = await Promise.all([
-              checkForChanges(currentDir),
+              checkDirForChanges(currentDir),
               fs.readFile(pathlib.join(currentDir, 'package.json'), 'utf-8')
                 .then(JSON.parse)
             ]);
 
             output[packageJson.name] = {
               directory: currentDir,
-              hasChanges,
+              hasChanges: packagesWithResolutionChanges?.has(packageJson.name) ?? hasChanges,
               package: packageJson
             };
           } catch (error) {
@@ -93,16 +106,18 @@ export function processRawPackages(topoOrder: string[], packages: Record<string,
     if (!packageInfo.hasChanges) {
       if (packageInfo.package.dependencies) {
         for (const name of Object.keys(packageInfo.package.dependencies)) {
-          if (packages[name].hasChanges) {
+          if (packages[name]?.hasChanges) {
             packageInfo.hasChanges = true;
             break;
           }
         }
       }
 
+      // If hasChanges still hasn't been set yet, we can proceed to iterate
+      // through devDependencies as well
       if (!packageInfo.hasChanges && packageInfo.package.devDependencies) {
         for (const name of Object.keys(packageInfo.package.devDependencies)) {
-          if (packages[name].hasChanges) {
+          if (packages[name]?.hasChanges) {
             packageInfo.hasChanges = true;
             break;
           }
@@ -203,15 +218,14 @@ function setOutputs(
   devserver: PackageRecord,
   docserver: PackageRecord
 ) {
-  core.setOutput('bundles', bundles);
-  core.setOutput('tabs', tabs);
-  core.setOutput('libs', libs);
+  core.setOutput('bundles', bundles.filter(x => x.changes));
+  core.setOutput('tabs', tabs.filter(x => x.changes));
+  core.setOutput('libs', libs.filter(x => x.changes));
   core.setOutput('devserver', devserver);
   core.setOutput('docserver', docserver);
 }
 
-async function main() {
-  const gitRoot = await getGitRoot();
+export async function main() {
   const { packages, bundles, tabs, libs } = await getAllPackages(gitRoot);
 
   const { repository } = packageJson;
@@ -260,7 +274,7 @@ async function main() {
     packages['@sourceacademy/modules-docserver']
   );
 
-  const workflows = await checkForChanges(pathlib.join(gitRoot, '.github/workflows'));
+  const workflows = await checkDirForChanges(pathlib.join(gitRoot, '.github/workflows'));
   core.setOutput('workflows', workflows);
 }
 
@@ -269,6 +283,6 @@ if (process.env.GITHUB_ACTIONS) {
   try {
     await main();
   } catch (error: any) {
-    core.setFailed(error.message);
+    core.setFailed(error);
   }
 }

.github/actions/src/load-artifacts/__tests__/artifact.test.ts

@@ -1,3 +1,4 @@
+import { ArtifactNotFoundError } from '@actions/artifact';
 import * as core from '@actions/core';
 import * as exec from '@actions/exec';
 import * as manifest from '@sourceacademy/modules-repotools/manifest';
@@ -10,10 +11,10 @@ vi.mock(import('@actions/core'), async importOriginal => {
     ...original,
     // Mock these functions to remove stdout output
     error: vi.fn(),
-    info: () => {},
-    startGroup: () => {},
+    info: () => { },
+    startGroup: () => { },
     setFailed: vi.fn(),
-    endGroup: () => {}
+    endGroup: () => { }
   };
 });
 const mockedResolveAllTabs = vi.spyOn(manifest, 'resolveAllTabs');
@@ -41,6 +42,7 @@ test('tab resolution errors cause setFailed to be called', async () => {
 
   await main();
 
+  expect(mockedResolveAllTabs).toHaveBeenCalledOnce();
   expect(core.error).toHaveBeenCalledExactlyOnceWith('error1');
   expect(core.setFailed).toHaveBeenCalledExactlyOnceWith('Tab resolution failed with errors');
 });
@@ -68,22 +70,26 @@ test('tabs that can\'t be found are built', async () => {
     if (name === 'Tab0-tab') {
       return { artifact: { id: 0 } };
     }
-    throw new Error();
+    throw new ArtifactNotFoundError();
   });
 
   await main();
 
   expect(mockedGetArtifact).toHaveBeenCalledTimes(2);
+  expect(mockedResolveAllTabs).toHaveBeenCalledOnce();
+
   const [[artifactCall0], [artifactCall1]] = mockedGetArtifact.mock.calls;
   expect(artifactCall0).toEqual('Tab0-tab');
   expect(artifactCall1).toEqual('Tab1-tab');
 
   expect(exec.exec).toHaveBeenCalledTimes(2);
-  const [[,execCall0], [,execCall1]] = vi.mocked(exec.exec).mock.calls;
-  console.log('args are', execCall0);
+  const [[execCmd0, execCall0], [execCmd1, execCall1]] = vi.mocked(exec.exec).mock.calls;
+
+  expect(execCmd0).toEqual('yarn workspaces focus');
   expect(execCall0).toContain('@sourceacademy/tab-Tab1');
   expect(execCall0).not.toContain('@sourceacademy/tab-Tab0');
 
+  expect(execCmd1).toEqual('yarn workspaces foreach -pA');
   expect(execCall1).toContain('@sourceacademy/tab-Tab1');
   expect(execCall1).not.toContain('@sourceacademy/tab-Tab0');
 });
@@ -101,11 +107,12 @@ test('install failure means build doesn\'t happen', async () => {
     }
   });
 
-  mockedGetArtifact.mockRejectedValueOnce(new Error());
-
+  mockedGetArtifact.mockRejectedValueOnce(new ArtifactNotFoundError());
   mockedExec.mockResolvedValueOnce(1);
+
   await main();
 
+  expect(mockedGetArtifact).toHaveBeenCalledOnce();
   expect(exec.exec).toHaveBeenCalledOnce();
-  expect(core.setFailed).toHaveBeenCalledExactlyOnceWith('yarn workspace focus failed for Tab0');
+  expect(core.setFailed).toHaveBeenCalledExactlyOnceWith('yarn workspace focus failed');
 });