aws / aws-toolkit-vscode

Amazon Q, CodeCatalyst, Local Lambda debug, SAM/CFN syntax, ECS Terminal, AWS resources
https://marketplace.visualstudio.com/items?itemName=AmazonWebServices.amazon-q-vscode
Apache License 2.0
1.47k stars 408 forks source link

Add postMessage embedded APIs; set isEmbedded global #5283

Open AwsBrettBailey opened 2 months ago

AwsBrettBailey commented 2 months ago

Problem

sendMessage and receiveMessage VSCode proposed APIs, which are made available on VSCode for Web via a fork of the core code-oss project, need to be accessible to the AWS Toolkit. Additionally, Toolkit should be aware of when it is being run in an embedded context.

Solution

Detect if sendMessage and receiveMessage APIs are available on extension activate. If they are available, VSCode is running in a context which supports these two APIs.

In a future commit, we will add support for a unique auth strategy when embedded mode is detected. Sending this up as an initial PR, so I can learn the process and ensure that this is an acceptable base from which to start.

Changelog: I think this should not be required as there are not yet any user facing changes Test coverage: I added a minimal test in the style of web.test.ts Testing: I have some local changes that enable codewhisperer with an "embedded" credentials strategy, which is working as expected.

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

AwsBrettBailey commented 2 months ago

Fixing issue with the most recent commit.

AwsBrettBailey commented 2 months ago

Remaining test failure appear to be caused by replacement of the globals object after initialize is called. Copying my message from slack here for explanation of my blockers here:

Would someone from the toolkit team be available (hopefully today) to walk me through some challenges I'm facing testing the globals initialize function. The tests do some setup and make some assumptions in globalSetup.test.ts about the globals object. Here's the specific issues I'm facing as I understand them:

  • Tests are using this module scope globals object. This is intended for web mode where globals is shared. Calling initialize replaces this object, meaning I have to duplicate some setup of the globals object from global mocha beforeEach and afterEach hooks. I can work around this via that duplication, but it seems like the tests should be using the globals object from initialize in the first place, unless they are a web mode specific test.
  • As requested by comments on my above CR, I have moved detection for isEmbedded into initialize. Detection works by checkin the vscode.window object for sendMessage and receiveMessage methods. I would like to test cases where the methods are present and not present. This means that I need to be able to provide a different vscode.window object, or have the ability to modify the mocked window with mocks for those methods. vscode.window is configured and set to the result of getTestWindow in the global mocha hooks here.

Basically, what I'm saying is that the global setup for the tests seems to assume that globals has already been initialized and testing initialize in this framework is difficult. It doesn't seem like there are sufficiently different test setups for web vs non-web, or the ability to opt-in or out of setup for the two modes.

I am still new to the test setup, so maybe I am overthinking this and there is a simpler solution, but I don't think it is in scope of my PR to modify the test setup substantially in order to test globals initialize and isEmbedded functionality.

nkomonen-amazon commented 2 months ago

@AwsBrettBailey regarding your latest comment. I think a simple solution could be to pass the vscode object as an argument to initialize(), similar to how we do it with isWeb. Additionally you could have it default to the vscode instance so the existing code does not need to change. Then in your tests you could pass in your modified vscode.

justinmk3 commented 3 weeks ago

CI is failing. Do you plan to continue working on this?

AwsBrettBailey commented 3 weeks ago

Yes, but I'm not sure when the next opportunity will be. If you would prefer to close, that is OK and we can reopen in the future. Otherwise, we will resume work here when we have a bit more bandwidth. Apologies for the delay.