Add support for test results summary and logs grouping via common-utils #70
Conversation
sameagen-MW
left a comment
sameagen-MW
left a comment
There was a problem hiding this comment.
LGTM!
There's a couple of comments / questions on my end, but it looks good overall. Thanks for working on this!
| @@ -0,0 +1,16 @@ | |||
| classdef GitHubLogPlugin < matlab.unittest.plugins.TestRunnerPlugin | |||
There was a problem hiding this comment.
Could we add Test into the name of the file somewhere so that it's more clear that this is a test runner plugin rather than a build runner plugin?
| import * as exec from "@actions/exec"; | ||
| import { matlab } from "run-matlab-command-action"; | ||
| import * as scriptgen from "./scriptgen"; | ||
| // TODO: update common-utils version when new version is released |
There was a problem hiding this comment.
Is this TODO still left to do later?
| @@ -0,0 +1,9 @@ | |||
| classdef GitHubLogPluginService < matlab.buildtool.internal.services.ciplugins.CITestRunnerPluginService | |||
There was a problem hiding this comment.
Does it make sense to ship and set up this plugin in the common component? That way tests always run with groupings on the output?
| # Download and extract in a temporary directory | ||
| WORKINGDIR=$(mktemp -d -t rmc_build.XXXXXX) | ||
| cd $WORKINGDIR | ||
| source ./node_modules/common-utils/scripts/setupdeps.sh |
There was a problem hiding this comment.
Hmm, this makes me a little bit nervous. I might almost rather have the same contents in both scripts rather than calling an upstream script. It feels like it makes it much more fragile, and I'm slightly worried about security concerns if that script happens to be hijacked.
That's just my preference though. I'll let others weigh in but I think I like it that the entire set up script is captured within the action itself.
| const pluginsPath = path.join(__dirname, "plugins").replaceAll("'","''"); | ||
| const command = "addpath('"+ pluginsPath +"'); " + scriptgen.generateCommand(options); |
There was a problem hiding this comment.
Can we extract this logic to somewhere else? It seems like it will likely have to be run by all of the actions to make sure the plugins get added correctly.
|
|
||
| await core.group("Run command", async () => { | ||
| await matlab.runCommand(helperScript, platform, architecture, exec.exec, startupOptions); | ||
| await matlab.runCommand(helperScript, platform, architecture, exec.exec, startupOptions).finally(() => { |
There was a problem hiding this comment.
Should we be setting the exec options (specifically MW_BATCH_LICENSING_ONLINE) here like we do in run-command?
This PR includes changes required with matlab-actions/common-utils#1 to begin using the new common utility.
NOTE: Before merging, the version for
common-utilsdependency inpackage.json,package-lock.jsonwill be updated after its release (couldn't add a comment since it's a JSON file). At the moment, the dependency refers to my working branch