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

[vscode] Add support for TestRunProfile onDidChangeProfile event #13388

Merged
merged 1 commit into from
Feb 28, 2024

Conversation

rschnekenbu
Copy link
Contributor

@rschnekenbu rschnekenbu commented Feb 14, 2024

What it does

This pull request add the support of API introduced with VS Code 1.86 on TestRunProfile, the onDidChangeDefault event.
It also adds the capability of changing default profile for a given test controller and a given kind of test run. The isDefault value is now updated as soon as the command "Select Default Test Profiles..." is used in the tool.

Fixes #13354

Contributed on behalf of STMicroelectronics

How to test

Install the following extension, mainly inspired from https://github.com/KevinClapperton/Test-API-functional-test

This extension will add a test controller and a command to create a test run profile, install a bunch of test and create a test run.

  1. When extension is installed, the "Test API: Run all tests" command can be invoked
  2. It will create 2 run profiles, and some tests. A couple of message windows will pop up.
  3. Then it is possible to switch between default profiles using the "Select Default Test Profiles..." in the command palette.
  4. The app should notify that isDefault has changed for the profiles impacted by the choice, i.e. the profiles from same controller that have the right kind as selected in the first pick dialog. Only one profile per controller and kind can be activated at once, on the contrary of VS code implementation. This latter implementation is a bug IMO, as only one profile will be chosen to run the tests, so having several default profiles is not relevant.

testing-default-profile

Note: the set isDefault value is not kept in settings, so user may need to configure it each time the app is started. This behavior is similar to the one on VS Code.

Follow-ups

There are no follow ups known currently.

Review checklist

Reminder for reviewers

@@ -2173,6 +2173,9 @@ export interface TestingExt {
/** Configures a test run config. */
$onConfigureRunProfile(controllerId: string, profileId: string): void;

/** Sets the default on a given run profile */
$onChangeDefault(controllerId: string, profileId: string, isDefault: boolean): void;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really correct according to our naming conventions ("onDidChange..."), but 1. it's consistent with the rest of the interface and 2. I invented it 🤦 .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the naming in favor of the onDid..., that seems indeed more inline with the usual naming in Theia.

private onDidChangeDefaultEmitter = new Emitter<boolean>();
onDidChangeDefault = this.onDidChangeDefaultEmitter.event;

protected notifyIsDefaultChange(_property: keyof TestRunProfileDTO, isDefault: boolean): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this will generate an extra call to the front end:

  1. Front set "isDefault"
  2. TestingExt.$onChangeDefault() is called
  3. notifyDefaultChanged is invoked and sends $updateTestRunProfile()

I don't think the "observableProperty" mechanism works for this: we need to distinguish where a change is coming from.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated to avoid this circular call. Only one call shall be made now.

@rschnekenbu
Copy link
Contributor Author

Hi @tsmaeder, thanks for your comments. I will solve conflicts and rebase to master once the review is done.

}

doSetDefault(isDefault: boolean): boolean {
let change = false;
Copy link
Contributor

@tsmaeder tsmaeder Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just

if (this._isDefault !== isDefault) {
            this._isDefault = isDefault;
            this.onDidChangeDefaultEmitter.fire(isDefault);
            return true;
        }
        return false; 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good one! Some remnants of old checkstyles rules :/ That will be fixed.

@tsmaeder
Copy link
Contributor

@rschnekenbu I'm not seeing the test controller after I install the attached extension. Also, does the ""Select default test Profile" se t the isDefault property on the profile from the extension?

@rschnekenbu
Copy link
Contributor Author

@tsmaeder, the extension is only activated when you run the command 'Test API: Run all tests'. Do you have any error on the console? There should be an entry in the log that indicates the extension was successful to create the controller ( SUCCESS TestController created: AllTestAPITestController )
The isDefault is not set from the extension. The extension reacts on change, but the change is changed from main.

@tsmaeder
Copy link
Contributor

@rschnekenbu I am having trouble testing this change: could you please give concrete steps how I change the default profile and how I can verify that a default profile is set? Secondly, we really should have a command that sets the default profile from the plugin side: otherwise this code path can't be verified.

@rschnekenbu
Copy link
Contributor Author

@tsmaeder, here is an updated version of the extension with a timeout function that sets the first profile to default after a small delay (~3sec). This will test the extension side of setting the default value.

For the steps:

  • Once the extension is installed, trigger from the command palette 'Test API: Run all Test'. This will create the test controller, test profiles, test items and test runs.
  • Once the command was run, you should have the AllTestAPITestController in the test explorer.
  • You should also get 2 notifications, one that will say that the testRunProfile could be created, and the second one that indicates that the profile.isDefault property was changed from the extension.
  • To change the default values, there is an entry on contextual menu of the TestItems "Select Default Test Profiles...". You can then select 'Run' or 'Debug' kind for the test profiles to configure. The list of test profiles with the right kind will show up. Any default test profile will have (default) append to their label. You can select the one you want to set as default.

add support to changing default profile in service
add event for isDefault onDidChangeProfile
add command to change default profile

fixes eclipse-theia#13354

Contributed on behalf of STMicroelectronics

Signed-off-by: Remi Schnekenburger <rschnekenburger@eclipsesource.com>

address review comments

reduce code
This was referenced Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[vscode] TestRunProfile now fires an event when isDefault is changed with 1.86
3 participants