Add per-project browser config processing #81

BTMorton · 2023-07-20T18:24:14Z

I finally got a chance to take another look at this (#71). Have hopefully addressed all of the initial feedback.

The browser helper classes are now all standalone, implementing the interface rather than extending an abstract base class.

It no longer executes the karma config file during extension configuration, but tries to figure out which browsers/custom launchers are supported by running regexp against the raw file as a string, as you've done in other places. The loader will then try to find a matching launch config from the user's final karma config when running the server and try to add the debug port flag. Unless a browser or custom launcher is set in the user's settings which will skip that part. I had to add a new env var to achieve this, though.

I've tested this with my target repo, a multi-project Angular + Electron app and it seems to be working as expected. The configuration defined in the karma.conf.js files are used by default, but can still be overriden in settings. Tests in all three projects can be run and debugged without issue.

The tests still need updating, but hopefully this is a step in the right direction to getting this functionality added. Let me know your thoughts.

… and debugger configurations Update extension config generation to load from project karma config files

Add user prefs to configure firefox to allow dev tools connections

…extend an abstract base class Update the config helper to avoid loading the karma config during configuration Update the config helper to look for the browser type for custom launch configs to determine if they are supported Update the karma config loader to use custom launcher configuration from the user's karma config if a matching config was found and it isn't overriden in settings

Add checks to ensure the custom launcher type is supported when compiling launcher configuration Update the electron helper to extend the chrome helper Update extension config tests to cover new browser/custom launcher config processing

BTMorton · 2023-07-21T11:50:18Z

Have updated the tests now, and expanded on the existing ones a little bit. Moving into ready for review as I think it's pretty much there, at least from my expectations.

lucono

This looks great @BTMorton. I had a couple of comments after which we can look to merge it. Thank you!

src/core/config/browsers/browser-helper.ts

src/core/config/browsers/electron-helper.ts

src/core/config/browsers/chrome-helper.ts

src/core/config/browsers/firefox-helper.ts

Update to pass config values to the browser helper rather than the whole config store Update to clone the custom launcher when adding debug configuration, rather than modifying in place

…r config in all helpers Update to check if the custom launcher base is supported as the first step in all custom launchers

lucono

@BTMorton this looks mostly good. I have a few comments and one question after testing, but those are minor enough that I can merge this and address them in a subsequent PR with other release updates. Thanks very much!

lucono · 2023-08-04T00:53:44Z

src/frameworks/karma/config/karma-config-loader.ts

+      if (!userSpecifiedLaunchConfig) {
+        customLaucherObject = this.findMatchingCustomLauncherFromConfig(customLaucherObject, config);
+      }


@BTMorton Why do we try to re-match this on the Karma process side? I imagine it could discard parts of the setup performed on the extension side, such as the headless/non-headless browser setup?

Since it no longer tries to process the karma config file on the extension side, it's much more challenging to pull out all of the custom launcher configuration. In order to make use of the user's launch config it needs to figure out if one is being used here. This puts an existing launch config slightly higher priority than the generated launch config as the user's config can be more specific. If the user specifies a browser or defines custom launch config in the extension settings, this step will be ignored. It was the best solution I could come up with at the time that achieved the goal of allowing custom launch configuration from the karma config file to be used by the extension.

To use a custom launch defined in the Karma config, I imagine you only need the name of that custom launcher, which you can place in the browsers prop of the config on the Karma process side. It seems your implementation already made a decent effort to obtain that from the extension process side of things and I'm pretty happy with the best effort that it makes there.

It checks if one of the custom launchers uses a supported browser and then pulls out that browser to setup debug configuration. Could make it pass through the custom launcher name as if it were the browser setting, but then that doesn't support the debug port assignments if you have multiple projects active. Would still ignore the headless/non-headless browser setup if that was configured, assuming you take the browser name first.

lucono · 2023-08-04T00:55:33Z

src/core/config/config-helper.ts

+ */
+export const getCustomLaunchConfiguration = (
+  config: ConfigStore<ProjectConfigSetting>,
+  projectKarmaConfigFilePath: string,


This should be string | undefined as starting with Angular 15, there isn't always a karma config file for each sub-project.

Could this fall back to the global karma config file path somehow?

I'm not familiar with a global Karma config, but I believe Angular 15+ uses an in-memory karma config, which the extension handles with karma.conf-default.ts. In the case of this update, I think it just needs to accommodate that the file can be absent and work with defaults in that scenario.

Ah, fair enough. I was thinking there was some kind of inheritance with library projects. That makes sense, though 👍

lucono · 2023-08-04T00:58:51Z

src/core/config/config-helper.ts

+  customLauncher: CustomLauncher | undefined;
+  userOverride: boolean;
+} => {
+  const rawConfig = getRawKarmaConfig(projectKarmaConfigFilePath, fileHandler);


In testing Angular 15+ scenarios where the Karma config file is not present on disk, this throws an exception trying to read a null file.

lucono · 2023-08-04T01:09:08Z

src/core/config/browsers/firefox-helper.ts

+      name: 'Karma Test Explorer Debugging',
+      type: this.debuggerType,
+      request: 'attach',
+      browserAttachLocation: 'workspace',
+      address: 'localhost',
+      port: FirefoxBrowserHelper.DEFAULT_DEBUGGING_PORT,
+      timeout: 60000


The browserAttachLocation, address, and timeout options don't seem to be valid for the Firefox debugger. For actual test debugging with breakpoint support, it also seems to require additional options.

I will admit to not testing this with Firefox... That's my bad 😬

lucono · 2023-08-04T01:27:25Z

src/core/config/browsers/firefox-helper.ts

+    return 'firefox';
+  }
+  public get debuggingPortFlag(): string {
+    return '-start-debugger-server';


Though this is right, it doesn't actually work for Firefox debugging because the -no-remote flag is hard-coded in the karma-firefox-launcher plugin.

lucono · 2023-08-04T01:29:42Z

src/core/config/extension-config.ts

@@ -144,9 +144,28 @@ export class ExtensionConfig implements Disposable {
    this.showTestDefinitionTypeIndicators = !!configStore.get(GeneralConfigSetting.ShowTestDefinitionTypeIndicators);
    this.debuggerConfigName = asNonBlankStringOrUndefined(configStore.get(GeneralConfigSetting.DebuggerConfigName));

+    const { browserType, customLauncher, userOverride } = getCustomLaunchConfiguration(
+      configStore,
+      this.projectKarmaConfigFilePath,


This can be null in the cases previously mentioned.

BTMorton and others added 3 commits July 20, 2023 17:57

Add browser-specific configuration helpers to process custom launcher…

9836c2b

… and debugger configurations Update extension config generation to load from project karma config files

Fix chrome's debugger flag to avoid it not starting

0377c30

Add user prefs to configure firefox to allow dev tools connections

BTMorton marked this pull request as draft July 20, 2023 18:31

Fix vscode test debugging config

e75d6a4

Add checks to ensure the custom launcher type is supported when compiling launcher configuration Update the electron helper to extend the chrome helper Update extension config tests to cover new browser/custom launcher config processing

BTMorton marked this pull request as ready for review July 21, 2023 11:50

lucono reviewed Jul 23, 2023

View reviewed changes

BTMorton added 2 commits July 26, 2023 18:48

Update to require at least one supported browser for each browser helper

2e28304

Update to pass config values to the browser helper rather than the whole config store Update to clone the custom launcher when adding debug configuration, rather than modifying in place

Update to check incoming browser type when creating a default launche…

e86621a

…r config in all helpers Update to check if the custom launcher base is supported as the first step in all custom launchers

lucono approved these changes Aug 4, 2023

View reviewed changes

lucono merged commit 04594f9 into lucono:master Aug 4, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add per-project browser config processing #81

Add per-project browser config processing #81

BTMorton commented Jul 20, 2023 •

edited

Loading

BTMorton commented Jul 21, 2023

lucono left a comment

lucono left a comment

lucono Aug 4, 2023

BTMorton Aug 4, 2023

lucono Aug 4, 2023

BTMorton Aug 4, 2023

lucono Aug 4, 2023

BTMorton Aug 4, 2023

lucono Aug 4, 2023

BTMorton Aug 4, 2023

lucono Aug 4, 2023

lucono Aug 4, 2023

BTMorton Aug 4, 2023

lucono Aug 4, 2023

lucono Aug 4, 2023

Add per-project browser config processing #81

Add per-project browser config processing #81

Conversation

BTMorton commented Jul 20, 2023 • edited Loading

BTMorton commented Jul 21, 2023

lucono left a comment

Choose a reason for hiding this comment

lucono left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BTMorton commented Jul 20, 2023 •

edited

Loading