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

fix(Settings): Use default Settings from Kaoto #709

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

lordrip
Copy link
Member

@lordrip lordrip commented Nov 15, 2024

Context

From the Kaoto side, we have default settings in the
settings.model.ts file.

Changes

This commit uses that and only overrides what needs to be changed in terms of the catalogUrl and the nodeLabel.

Notes

Once the VS Code setting for the new property nodeToolbarTrigger is merged, we can add it also to the VS Code settings object.

@lordrip lordrip requested review from apupier and djelinek November 15, 2024 14:47
Copy link
Member

@apupier apupier left a comment

Choose a reason for hiding this comment

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

it is fixing the compilation with main branch of kaoto
Several tests are broken with main branch of Kaoto but it sounds unrelated and could be tackled in another PR

@lordrip
Copy link
Member Author

lordrip commented Nov 15, 2024

it is fixing the compilation with main branch of kaoto Several tests are broken with main branch of Kaoto but it sounds unrelated and could be tackled in another PR

The failures are likely due to the changes in the canvas nodes, I'll check

@lordrip lordrip force-pushed the fix/use-default-settings branch from d4ffe5e to 01aea66 Compare November 15, 2024 20:14
Comment on lines 154 to 158
until.elementLocated(By.css('g[data-testid^="custom-node__timer"]'))
);
const threeDotsIconOfOneOfTheSteps = (await driver.findElements(By.xpath('//*[@data-type="node"]//*[@class="pf-topology__node__action-icon"]')))[1];
await threeDotsIconOfOneOfTheSteps.click();

const canvasNode = await driver.findElement(By.css('g[data-testid^="custom-node__timer"]'));
driver.actions().contextClick(canvasNode).perform();
Copy link
Member Author

Choose a reason for hiding this comment

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

Since the 3-dots button is no longer there, we're triggering a right-click event over the node to get the same menu.

Copy link
Contributor

Choose a reason for hiding this comment

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

just a small one for the stabillity.. is the "driver.actions...perform()" a floating Promise? I think we could missing 'await' here

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know, I'll check

Copy link
Member Author

Choose a reason for hiding this comment

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

Alrighty, I was missing the await, perform is certainly a Promise, thanks for the catch @djelinek

timer: By.xpath(`//\*[name()='g' and starts-with(@data-id,'timer')]`),
label: By.xpath(`//\*[name()='g' and starts-with(@class,'pf-topology__node__label')]`)
timer: `g[data-id^='timer'][data-kind='node']`,
label: `.custom-node__label`,
Copy link
Member Author

Choose a reason for hiding this comment

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

This selector changed because there's no more topology label but rather a standard text.

@@ -64,7 +64,7 @@ describe('User Settings', function () {
it(`Check 'id' Node Label is used instead of default 'description'`, async function () {
this.timeout(60_000);

const timer = await driver.findElement(locators.TimerComponent.timer).findElement(locators.TimerComponent.label);
const timer = await driver.findElement(By.css(`${locators.TimerComponent.timer} ${locators.TimerComponent.label}`));
Copy link
Member Author

Choose a reason for hiding this comment

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

Semantically, this is not exactly the same as before.

Before, we were locating a node and from that point, we searched for another one within. Now, we're targeting the label that is inside of the node.

The issue is that for some reason, topology now creates invisible nodes, potentially as references inside of the groups, so it means the test library finds 2 nodes, instead of one, and the first one is empty, therefore, the location fails.

From Kaoto side, we have default settings in the
[settings.model.ts](https://github.com/KaotoIO/kaoto/blob/9ee3368e0e488853f61a84fa4e16bedadc801fda/packages/ui/src/models/settings/settings.model.ts#L23-L25)
file.

This commit uses that and only override what needs to be changed in
terms of the `catalogUrl` and the `nodeLabel`.

Once the VS Code setting for the new property `nodeToolbarTrigger` is merged, we can add it also to the VS Code settings object.
@lordrip lordrip force-pushed the fix/use-default-settings branch from 01aea66 to 35baa2f Compare November 18, 2024 07:36
@apupier apupier merged commit 314a751 into KaotoIO:main Nov 18, 2024
6 checks passed
@lordrip lordrip deleted the fix/use-default-settings branch November 18, 2024 08:44
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.

3 participants