Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v0.9.0 update #83

Closed
wants to merge 2 commits into from
Closed

v0.9.0 update #83

wants to merge 2 commits into from

Conversation

lucono
Copy link
Owner

@lucono lucono commented Aug 5, 2023

@lucono
Copy link
Owner Author

lucono commented Aug 5, 2023

@BTMorton Do you mind verifying that the Electron use-case still works for you as expected with these modifications before we release it? Given that Karma is now deprecated, this might be the last feature update. Thanks.

@BTMorton
Copy link
Contributor

BTMorton commented Aug 7, 2023

Nope, it tries to run with Electron, but doesn't load the custom launcher options defined in my karma config. That's why I had the attempt to match the browser type with a custom launcher in the karma config

@lucono
Copy link
Owner Author

lucono commented Aug 7, 2023

Does it work when you simply reference the custom launcher name in the browser setting of the extension? In that scenario, I expect it should go through your enhancements on the extension-side to parse the custom launcher entry from the karma config and use its base value to find the right browser helper.

@BTMorton
Copy link
Contributor

BTMorton commented Aug 8, 2023

It would work for that one project, but the other two projects which don't use electron have a different custom launcher name. I could set all three to have the same name, but the issue then would be that the debugger would be listening on the same port for all three because it would be using the browser setting and not applying the debug port setting.

One option could be to pass the custom launcher name pulled out of the karma config through to the karma config loader, then load up that custom launcher and set the debug port, or combine the custom launchers or something.

The debate essentially is what do we want to respect more: the user's karma config custom launcher settings, or the extension setting to show the browser window. IMO, if we're detecting the user's custom launcher we should just respect those settings and have the extension options only apply when constructing the launcher for a supported browser. But that's my opinion as someone who uses a custom launcher and doesn't use the extension settings :P

@lucono
Copy link
Owner Author

lucono commented Aug 8, 2023

It would work for that one project, but the other two projects which don't use electron have a different custom launcher name. I could set all three to have the same name, but the issue then would be that the debugger would be listening on the same port for all three because it would be using the browser setting and not applying the debug port setting.

You're right, that's a problem.

IMO, if we're detecting the user's custom launcher we should just respect those settings and have the extension options only apply when constructing the launcher for a supported browser

The way I think about it is that the extension doesn't have to conform to the Karma config's exact launcher settings, but should try and use the same browser as the launcher for running the tests. That's because the extension should be able to apply a launcher that best allows it to run and debug the tests within VSCode. One example is the fact that it disables every other reporter configured in the project, except for its own reporter, in order to improve performance, and because those reporters would probably not be required in the vscode test execution scenario. Another is its need to be able to adapt the setup to run in a container if it detects the tests are running in one. The other, most obvious one, is enabling debugging, and/or adjusting its port.

My thinking is that if the user sets a browser in the extension settings, then it should honor that and perhaps just try to configure the debugger port if it's a supported browser. (Before your enhancements, it would just honor the specified browser and give up entirely on any attempt to configure the debugger port).

One option could be to pass the custom launcher name pulled out of the karma config through to the karma config loader, then load up that custom launcher and set the debug port

This makes sense to me. Perhaps only the user's browser setting would be required in that case, 'cause I think in the current PR, in the case that a customLauncher is provided, the debugger port is already being added (but not in the case where the browser is provided).

Finally, regarding this again:

It would work for that one project, but the other two projects which don't use electron have a different custom launcher name. I could set all three to have the same name, but the issue then would be that the debugger would be listening on the same port for all three because it would be using the browser setting and not applying the debug port setting.

..do you think adding GeneralConfigSetting.Browser and GeneralConfigSetting.CustomLauncher to project-specific-config.ts would be a good way to resolve that by adding those settings at a project level, since they're then overlayed on top of the more general settings?

@BTMorton
Copy link
Contributor

BTMorton commented Aug 8, 2023

..do you think adding GeneralConfigSetting.Browser and GeneralConfigSetting.CustomLauncher to project-specific-config.ts would be a good way to resolve that by adding those settings at a project level, since they're then overlayed on top of the more general settings?

Yes, that's a pretty good solution, particularly if it could work in partnership with applying the debugger port to a configured browser setting.

The way I think about it is that the extension doesn't have to conform to the Karma config's exact launcher settings, but should try and use the same browser as the launcher for running the tests. That's because the extension should be able to apply a launcher that best allows it to run and debug the tests within VSCode. One example is the fact that it disables every other reporter configured in the project, except for its own reporter, in order to improve performance, and because those reporters would probably not be required in the vscode test execution scenario. Another is its need to be able to adapt the setup to run in a container if it detects the tests are running in one. The other, most obvious one, is enabling debugging, and/or adjusting its port.

That's fair enough. Perhaps a compromise could be to add a config setting that does just try to use the any custom configureation from the karma config, with the explicit caveat that you don't get any of these benefits (excluding the debugger port because that's already being added easily enough).

@lucono
Copy link
Owner Author

lucono commented Aug 12, 2023

@BTMorton I've re-added the logic where it tries to match and identify the right custom launcher on the Karma side in order to apply the right port for debugging, for each project. Also added project-specific browser and customLauncher settings, though realized they will only help in the scenario of separate projects in a monorepo, but not sub-projects in a single Angular workspace. That's because project-specific settings aren't currently resolved down to the sub-project level.

In its current state, the updated extension attempts to identify and use the right browser for testing and debugging, but does not try to use the specific launcher settings in the project config (as discussed earlier in the thread). Not perfect but a huge improvement.

In testing, I'm able to debug 3 sub-projects using 3 different browsers in the same Angular workspace, and without any extension configuration for browser or custom launchers. It accurately detects them from each of their Karma configs. I imagine this is the closest we can come without other major changes, and without breaking any other setup performed on the extension side, such as headless/non-headless.

image

@BTMorton
Copy link
Contributor

So, is the expectation then that to use a custom launcher from your karma config, you need to specify it as the browser setting? It'll then do the same as previously, but also add the debugger port configuration. It'd be nice if there was a setting that was to try to use custom launcher config from the karma config file, which then doesn't require you to change any other config, but I can probably make this solution work.

@lucono
Copy link
Owner Author

lucono commented Aug 17, 2023

is the expectation then that to use a custom launcher from your karma config, you need to specify it as the browser setting? It'll then do the same as previously, but also add the debugger port configuration

Yes.

This update would add two main enhancements:

  1. Automatic zero-configuration debugging support for Firefox and Chromium browsers (in addition to Chrome) to use one of the project's own selected browsers in its karma config
  2. Debugging support when a custom launcher or browser is specified in the extension settings by adding a debug port, which wasn't previously supported

But it doesn't provide a way to use the exact custom launcher setup in a project's karma config.

Given the newly deprecated status of Karma, I think the new updates strike a balance for adding basic multi-browser support without too much additional investment in new features. The focus will now be on mostly maintenance and stability.

@lucono
Copy link
Owner Author

lucono commented Sep 5, 2023

@BTMorton Apologies, I'v been away. Please see my comment here given the deprecation of Karma for over 4 months at this point, especially given that the updates and all previous iterations in this PR remain available for use outside of the marketplace extension. Thank you!

@BTMorton
Copy link
Contributor

That's all fine with me. I can work with what's here. Thanks for your help!

@lucono lucono closed this Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants