Conversation
There was a problem hiding this comment.
Pull request overview
This pull request introduces a JavaScript-based implementation of the TRX to VS Playlist Converter action alongside the existing composite action. The JavaScript action uses Node.js with a pre-install step to set up the trx-to-vsplaylist .NET tool, then runs the conversion in the main step.
Changes:
- Adds JavaScript action implementation in
js/directory with pre.js (tool installation) and main.js (conversion logic) - Moves original composite action to
composite/subdirectory to maintain backward compatibility - Updates main action.yml to use the new JavaScript implementation
- Updates tests and workflows to reference the composite action explicitly where needed
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| js/src/pre.js | Pre-step script that installs the trx-to-vsplaylist .NET tool |
| js/src/main.js | Main script that runs the TRX to playlist conversion |
| js/rollup.config.mjs | Rollup bundler configuration for building the JavaScript action |
| js/package.json | npm package configuration with dependencies |
| js/action.yml | Action metadata for the standalone JS action |
| action.yml | Main action metadata updated to use the JavaScript implementation |
| composite/action.yml | Original composite action moved to subdirectory |
| README.md | Documentation updated to reflect JavaScript action as default with composite as legacy option |
| .github/workflows/update-trx-to-vsplaylist.yml | Workflow updated to reference composite/action.yml |
| .github/workflows/test.yml | Tests updated to explicitly test both composite and JavaScript implementations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // After running, set outputs similar to the composite action | ||
| if (separate === 'true') { | ||
| const playlistFiles = await globAsync('**/*.playlist', { cwd: artifactDir, absolute: true }) |
There was a problem hiding this comment.
Similar to line 8, the glob function in v11 already returns a promise and should not be wrapped with promisify. Use glob directly: const playlistFiles = await glob('**/*.playlist', { cwd: artifactDir, absolute: true })
| run: | | ||
| sed -i "s/dotnet tool install --global trx-to-vsplaylist --version [0-9.][0-9.]*/dotnet tool install --global trx-to-vsplaylist --version ${{ steps.get_version.outputs.latest }}/g" action.yml | ||
| sed -i "s/dotnet tool install --global trx-to-vsplaylist --version [0-9.][0-9.]*/dotnet tool install --global trx-to-vsplaylist --version ${{ steps.get_version.outputs.latest }}/g" composite/action.yml | ||
|
|
There was a problem hiding this comment.
The update workflow now only updates the composite/action.yml file but not the js/src/pre.js file which also has the hardcoded version 1.3.0. The workflow should be updated to also update the version in js/src/pre.js to keep them in sync, or the pre.js should be modified to not hardcode the version.
| - name: Update js/src/pre.js with new version | |
| if: steps.check.outputs.update == 'true' | |
| run: | | |
| # Replace the first semantic version occurrence (e.g., 1.3.0) with the latest version | |
| sed -i -E "0,/([0-9]+\.[0-9]+\.[0-9]+)/s//${{ steps.get_version.outputs.latest }}/" js/src/pre.js |
| @@ -125,6 +127,30 @@ You can specify multiple outcomes by separating them with commas: `Failed,NotExe | |||
|
|
|||
| The generated playlist file is automatically uploaded as a workload artifact. | |||
There was a problem hiding this comment.
The README states "The generated playlist file is automatically uploaded as a workload artifact" but the JavaScript action doesn't include artifact upload functionality. This is a discrepancy with the documented behavior. Either update the documentation to clarify that the JS action doesn't upload artifacts automatically and users need to add their own artifact upload step, or implement artifact upload functionality similar to the composite action.
| const child = exec(command, { windowsHide: true }, (error, stdout, stderr) => { | ||
| if (stdout) process.stdout.write(stdout) | ||
| if (stderr) process.stderr.write(stderr) | ||
| if (error) { | ||
| reject(error) | ||
| } else { | ||
| resolve() | ||
| } | ||
| }) |
There was a problem hiding this comment.
The child variable is declared but never used after being assigned. The exec callback handles all the output and error logic, so the child process reference is unnecessary. Remove the 'const child = ' assignment and just call exec directly.
| import { promisify } from 'node:util' | ||
| import { glob } from 'glob' | ||
| import { dirname, basename, join } from 'node:path' | ||
| import { existsSync } from 'node:fs' | ||
|
|
||
| const globAsync = promisify(glob) | ||
|
|
There was a problem hiding this comment.
The glob package v11 already returns a promise, so wrapping it with promisify is incorrect. The glob function should be used directly without promisify. Change this to directly use the glob function: const trxFiles = await glob(trxPattern) and remove the promisify import and this line.
| import { promisify } from 'node:util' | |
| import { glob } from 'glob' | |
| import { dirname, basename, join } from 'node:path' | |
| import { existsSync } from 'node:fs' | |
| const globAsync = promisify(glob) | |
| import { glob } from 'glob' | |
| import { dirname, basename, join } from 'node:path' | |
| import { existsSync } from 'node:fs' |
| if (outputDirectory) { | ||
| artifactDir = outputDirectory | ||
| } else { | ||
| artifactDir = dirname(trxFiles[0]) | ||
| } |
There was a problem hiding this comment.
When an output directory is specified by the user, it should be created if it doesn't exist, similar to how the composite action uses mkdir -p. Without this, the action will fail if the user specifies a non-existent directory. Add a check to create the directory using Node.js fs.mkdirSync with recursive option when outputDirectory is provided.
| async function run() { | ||
| try { | ||
| core.info('Setting up .NET and installing trx-to-vsplaylist tool (pre step)...') | ||
|
|
||
| // Match the composite action: setup .NET 10.x GA and install the tool globally | ||
| await runCommand('dotnet --info') | ||
|
|
||
| await runCommand('dotnet tool install --global trx-to-vsplaylist --version 1.3.0 --source https://api.nuget.org/v3/index.json') |
There was a problem hiding this comment.
The pre.js runs dotnet --info and installs the .NET tool without first setting up .NET, unlike the composite action which uses actions/setup-dotnet@v5 to set up .NET 10.x. If the GitHub Actions runner doesn't have .NET installed or has an incompatible version, this will fail. Consider documenting this requirement or adding logic to handle missing .NET installations gracefully.
| async function run() { | |
| try { | |
| core.info('Setting up .NET and installing trx-to-vsplaylist tool (pre step)...') | |
| // Match the composite action: setup .NET 10.x GA and install the tool globally | |
| await runCommand('dotnet --info') | |
| await runCommand('dotnet tool install --global trx-to-vsplaylist --version 1.3.0 --source https://api.nuget.org/v3/index.json') | |
| async function ensureDotnetAvailable(): Promise<boolean> { | |
| try { | |
| await runCommand('dotnet --info') | |
| return true | |
| } catch (error) { | |
| // Handle the specific case where the dotnet executable is not found. | |
| if (error && typeof error === 'object' && 'code' in error && error.code === 'ENOENT') { | |
| core.setFailed( | |
| '`.NET (dotnet)` is not installed or not available on PATH. ' + | |
| 'Please add an `actions/setup-dotnet@v5` step to your workflow to install .NET 10.x before this action runs.' | |
| ) | |
| return false | |
| } | |
| throw error | |
| } | |
| } | |
| async function run() { | |
| try { | |
| core.info('Setting up .NET and installing trx-to-vsplaylist tool (pre step)...') | |
| // Ensure dotnet is available before proceeding with tool installation | |
| const hasDotnet = await ensureDotnetAvailable() | |
| if (!hasDotnet) { | |
| return | |
| } | |
| await runCommand( | |
| 'dotnet tool install --global trx-to-vsplaylist --version 1.3.0 --source https://api.nuget.org/v3/index.json' | |
| ) |
| import { promisify } from 'node:util' | ||
| import { glob } from 'glob' | ||
| import { dirname, basename, join } from 'node:path' | ||
| import { existsSync } from 'node:fs' | ||
|
|
||
| const globAsync = promisify(glob) | ||
|
|
There was a problem hiding this comment.
The promisify import is no longer needed since glob v11 already returns promises. This import should be removed along with the globAsync promisification on line 8.
| import { promisify } from 'node:util' | |
| import { glob } from 'glob' | |
| import { dirname, basename, join } from 'node:path' | |
| import { existsSync } from 'node:fs' | |
| const globAsync = promisify(glob) | |
| import { glob } from 'glob' | |
| import { dirname, basename, join } from 'node:path' | |
| import { existsSync } from 'node:fs' |
| </TestRun> | ||
| EOF | ||
|
|
||
| - name: Test Action - Merge multi-framework results | ||
| - name: Test Action - Merge multi-framework results (JS default) | ||
| uses: ./ | ||
| with: | ||
| trx-file-path: './TestResults/net*.trx' |
There was a problem hiding this comment.
The test passes an 'artifact-name' input to the JS action, but this input is not defined in js/action.yml or action.yml. The JS action only supports the inputs: trx-file-path, output-directory, test-outcomes, skip-empty, and separate. This test should either remove the artifact-name parameter or the JS action should support it if artifact upload functionality is added.
| exit 1 | ||
| fi | ||
|
|
||
| - name: Test Action - Separate multi-framework results | ||
| - name: Test Action - Separate multi-framework results (JS default) | ||
| uses: ./ | ||
| with: | ||
| trx-file-path: './TestResults/net*.trx' |
There was a problem hiding this comment.
The test passes an 'artifact-name' input to the JS action, but this input is not defined in js/action.yml or action.yml. The JS action only supports the inputs: trx-file-path, output-directory, test-outcomes, skip-empty, and separate. Remove the artifact-name parameter from this test.
No description provided.