- Notifications
You must be signed in to change notification settings - Fork515
Un-register extension between steps to prevent cascading test failures#5227
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Un-register extension between steps to prevent cascading test failures#5227
Conversation
…ting all registered extensions.- Update ensureEditorServicesIsConnected test method to un-register the extension if its already registered to prevent cascading test failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
This PR prevents cascading test failures by ensuring that any previously registered extension is unregistered before a new registration occurs.
- Adds a new getRegisteredExtensions method to the extension client.
- Updates the test to loop over registered extensions and unregister any that match the target extension ID.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/utils.ts | Updated test logic to unregister a pre-registered extension. |
| src/features/ExternalApi.ts | Added getRegisteredExtensions to expose registered extensions. |
| src/extension.ts | Extended the client wrapper to include the new getRegisteredExtensions method. |
Comments suppressed due to low confidence (1)
test/utils.ts:85
- The variable name '_name' is ambiguous. Consider renaming it to 'uuid' or 'extensionKey' to better convey its purpose.
for (const [| if (externalExtension.id === extensionId) { | ||
| extension.unregisterExternalExtension(_name); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[nitpick] There is an unnecessary semicolon after the for loop block; removing it would improve code readability.
| }; | |
| } |
Copilot uses AI. Check for mistakes.
@tsmarvin is there a particular issue this is fixing in the external API you are seeing? |
tsmarvin commentedJul 1, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Sort of. I added this so that it was easier for me to debug the actual root cause issue I was having. I added it pretty early on in my testing so I'm not 100% confident on why I was seeing the cascading registration errors, but i suspect the PSES internal host not starting left the test waiting for the connection and it skipped over the de-registration on timeout. If you don't think it's necessary I won't be offended, just figured id submit the PR as it helped me personally when debugging. It seemed like it was the expected state for the start of several tests related to the method I added it to. |
PR Summary
PR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
xbetween the square brackets.Please mark anything not applicable to this PR
NA.WIP:to the beginning of the title and remove the prefix when the PR is ready