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

preferences: fix duplicated call to FoldersPreferencesProvider #12174

Merged
merged 3 commits into from
Mar 23, 2023

Conversation

paul-marechal
Copy link
Member

@paul-marechal paul-marechal commented Feb 10, 2023

Properly inhibit preference providers when they shouldn't apply during resolution.

How to test

Debug the following method:

protected doResolve<T>(preferenceName: string, defaultValue?: T, resourceUri?: string): PreferenceResolveResult<T> {
const result: PreferenceResolveResult<T> = {};
for (const scope of PreferenceScope.getScopes()) {
if (this.schema.isValidInScope(preferenceName, scope)) {
const provider = this.getProvider(scope);
if (provider?.canHandleScope(scope)) {
const { configUri, value } = provider.resolve<T>(preferenceName, resourceUri);
if (value !== undefined) {
result.configUri = configUri;
result.value = PreferenceProvider.merge(result.value as any, value as any) as any;
}
}
}
}
return {
configUri: result.configUri,
value: result.value !== undefined ? deepFreeze(result.value) : defaultValue
};
}

Make sure it doesn't end up calling the same provider twice.

Review checklist

Reminder for reviewers

@paul-marechal paul-marechal changed the title Mp/fix duplicated provider2 preference: fix duplicated call to FoldersPreferencesProvider Feb 10, 2023
@paul-marechal paul-marechal changed the title preference: fix duplicated call to FoldersPreferencesProvider preferences: fix duplicated call to FoldersPreferencesProvider Feb 10, 2023
@paul-marechal paul-marechal force-pushed the mp/fix-duplicated-provider2 branch 2 times, most recently from 62e23c7 to 7971705 Compare February 10, 2023 20:27
@paul-marechal paul-marechal marked this pull request as ready for review March 6, 2023 15:41
@paul-marechal
Copy link
Member Author

@AlexandraBuzila can you take over fixing the test suite? This is what you forgot to do the first time you introduced the change to merge configs together: The tests must be updated accordingly.

@AlexandraBuzila
Copy link
Contributor

AlexandraBuzila commented Mar 7, 2023

Hi @paul-marechal! Yes, I can have a look at the tests.
Just to clarify, this is about reapplying the changes from #12126 and fixing the tests, correct? The tests on this branch are green on my end.

Unrelated question: I was able to run the tests on your branch locally, but after reapplying my changes I keep getting an NSFW service error and the tests are pending.

root ERROR [nsfw-watcher: 49885] NSFW service error on "/tmp/theia-test-config-dir202327-49736-3feycm.32qeo": [Error: Service shutdown: root path changed (renamed or deleted)]

I had this happen in the past as well, but it resolved itself after a few retries. Now I can't get them to work at all. Do you maybe know if there's a trick to fix this? I'm running on linux.

@paul-marechal
Copy link
Member Author

paul-marechal commented Mar 7, 2023

I think the launch test suite is currently being skipped. edit: They're not.

We're indeed missing the the changes from #12126

@AlexandraBuzila
Copy link
Contributor

Hi @paul-marechal
I added back the changes from #12126 and fixed the tests in #12275

The target for that PR is this branch, as your changes are needed as well for the tests to work.

@vince-fugnitto vince-fugnitto added the preferences issues related to preferences label Mar 13, 2023
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes look good to me 👍

examples/api-tests/src/launch-preferences.spec.js Outdated Show resolved Hide resolved
paul-marechal and others added 3 commits March 23, 2023 13:44
This is useful to disable the `PreferenceScope.Folders` scope when it
no longer applies upon preference resolution.
This reverts commit a3b9735 and fixes
the launch-preferences tests
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@paul-marechal paul-marechal merged commit 656cd9b into master Mar 23, 2023
@paul-marechal paul-marechal deleted the mp/fix-duplicated-provider2 branch March 23, 2023 19:43
@github-actions github-actions bot added this to the 1.36.0 milestone Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preferences issues related to preferences
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants