Fix/argument parsing issues 31

jacobwi commented 5 months ago

Summary

This pull request addresses argument parsing issues identified in issue #31, where --config and --label options were not being correctly processed. Additionally, it includes updates to dependencies and applies Prettier formatting across the codebase.

Changes

Fixed argument parsing logic for --config and --label options to correctly interpret command-line inputs.
Applied Prettier formatting to ensure consistent coding styles throughout the project.
Updated various dependencies to their latest versions to incorporate security patches and performance improvements.

Motivation

These changes were prompted by user-reported issues with command-line argument parsing that hindered the tool's usability. By resolving these issues, enhancing code readability through standardized formatting, and updating dependencies, this pull request aims to improve both the developer experience and the tool's overall security and stability.

Testing

Manual testing was conducted to verify that the --config and --label options are now correctly parsed and processed.
Automated tests were updated to reflect changes in argument parsing logic.
Dependency updates were tested to ensure no regressions in functionality.

Review Notes

Special attention should be given to the argument parsing function to ensure all edge cases are handled.
Reviewers may want to pull the branch and conduct their own testing, particularly around the updated dependencies to catch any potential incompatibilities.

jacobwi commented 5 months ago

@microsoft-github-policy-service agree

connor4312 commented 5 months ago

I'm not clear on how this affects the parsing or handling of label, could you explain more about what the issue/fix is?

jacobwi commented 5 months ago

Hey @connor4312 ,

I will start with stacktrace:

C:\Users\jacob\OneDrive\Desktop\Code\context-menu-command\commands-menu\node_modules.pnpm\enhanced-resolve@5.16.0\node_modules\enhanced-resolve\lib\util\path.js:41 switch (p.length) { ^

TypeError: Cannot read properties of undefined (reading 'length')
    at getType (C:\Users\jacob\OneDrive\Desktop\Code\context-menu-command\commands-menu\node_modules\.pnpm\enhanced-resolve@5.16.0\node_modules\enhanced-resolve\lib\util\path.js:41:12)
    at join (C:\Users\jacob\OneDrive\Desktop\Code\context-menu-command\commands-menu\node_modules\.pnpm\enhanced-resolve@5.16.0\node_modules\enhanced-resolve\lib\util\path.js:153:10)
    at cachedJoin (C:\Users\jacob\OneDrive\Desktop\Code\context-menu-command\commands-menu\node_modules\.pnpm\enhanced-resolve@5.16.0\node_modules\enhanced-resolve\lib\util\path.js:191:15)
    at Resolver.join (C:\Users\jacob\OneDrive\Desktop\Code\context-menu-command\commands-menu\node_modules\.pnpm\enhanced-resolve@5.16.0\node_modules\enhanced-resolve\lib\Resolver.js:784:10)
    at C:\Users\jacob\OneDrive\Desktop\Code\context-menu-command\commands-menu\node_modules\.pnpm\enhanced-resolve@5.16.0\node_modules\enhanced-resolve\lib\ModulesInHierarchicalDirectoriesPlugin.js:41:50
    at Array.map (<anonymous>)
    at C:\Users\jacob\OneDrive\Desktop\Code\context-menu-command\commands-menu\node_modules\.pnpm\enhanced-resolve@5.16.0\node_modules\enhanced-resolve\lib\ModulesInHierarchicalDirectoriesPlugin.js:41:32
    at Array.map (<anonymous>)
    at C:\Users\jacob\OneDrive\Desktop\Code\context-menu-command\commands-menu\node_modules\.pnpm\enhanced-resolve@5.16.0\node_modules\enhanced-resolve\lib\ModulesInHierarchicalDirectoriesPlugin.js:40:14
    at _next0 (eval at create (C:\Users\jacob\OneDrive\Desktop\Code\context-menu-command\commands-menu\node_modules\.pnpm\tapable@2.2.1\node_modules\tapable\lib\HookCodeFactory.js:33:10), <anonymous>:8:1)

Node.js v21.7.1

It works because it is able to find the default config and labelprofile if you're running it without parameters i.g. 'vscode-test' because the path isn't relative and it flows from loadDefaultConfigFile -> to the following statement in tryLoadConfigFile ->

   let loaded = await configFileRules[ext](path <- **this path is absolute here**);
        if ('default' in loaded) {
            // handle default es module exports
            loaded = loaded.default;
        }

However, now add --config and --label

the config obj in main is:

passing it to runConfigs()

and then to const prepared = await prepareConfigs(config, enabledTests) to prepare the platform

  const p = await platform.prepare({ args, config, test });

....

// PreparedDesktopRun will do its thing then it will trigger runPreparedConfigs()

Where it crashes:

async function runPreparedConfigs(config, prepared) {
    const coverage = args.coverage ? new Coverage(config, args) : undefined;
    const context = { coverage: coverage?.targetDir };
    let code = 0;
    for (const p of prepared) {

// this will import electron tests-> code = Math.max(code, await p.run(context));
        if (args.bail && code !== 0) {
            return code;
        }
    }
    await coverage?.write();
    return code;
}

    async importTestElectron() {
continues here ->        const electronPath = await mustResolve(this.config.dir, '@vscode/test-electron');
        const electron = await import(pathToFileURL(electronPath).toString());
        return electron;
    }

it calls commonJsResolve which is promisifed const of resolveCb of enhanced-resolve

export const mustResolve = async (context, moduleName) => {
CRASH ->    const path = await commonJsResolve(context, moduleName);
    if (!path) {
        let msg = `Could not resolve module "${moduleName}" in ${path}`;
        if (!moduleName.startsWith('.')) {
            msg += ' (you may need to install with `npm install`)';
        }
        throw new CliExpectedError(msg);
    }
    return path;
};

absolute vs relative (not passing --config vs. passing a --config)

vs.

Fix applied (resolve it to absolute before creating overhead and propagating it down so it can resolve config file and test set with its label):

tryLoadConfigFile() ->

if (!isAbsolute(path)) {
    path = resolvePath(path);
}

microsoft / vscode-test-cli