introduce invalid[Edge].spec.ts #24

jomarko · 2024-03-12T15:58:14Z

No description provided.

packages/dmn-editor/tests/e2e/drgRequirements/invalidAuthorityRequirement.spec.ts

jomarko · 2024-03-13T11:18:11Z

packages/dmn-editor/tests/e2e/drgRequirements/invalidInformationRequirement.spec.ts

+    test("shouldn't add an Information Requirement edge from Input Data node to BKM node", async ({
+      palette,
+      edges,
+      nodes,
+    }) => {
+      await palette.dragNewNode({
+        type: NodeType.BKM,
+        targetPosition: { x: 300, y: 100 },
+      });
+
+      await nodes.dragNewConnectedEdge({
+        type: EdgeType.INFORMATION_REQUIREMENT,
+        from: "Source Node",
+        to: DefaultNodeName.BKM,
+      });
+
+      expect(await edges.get({ from: "Source Node", to: DefaultNodeName.BKM })).not.toBeAttached();
+    });


I put the comment here, but it is general for all tests. Currently, each test checks only the DOM structural changes.

However, we could also check the visual effect. Once the user starts to drag edge, non-valid node targets are dimmed. We can say disabled. however, in code, the change is represented only by the change in the css class. If I am correct, we want to avoid using similar internal changes, non-visible for users for writing tests. So I am not sure if we should test also this visual effect or not, please share your opinion.

Please, notice the div class in the highlighted html source code row.

Non dimmed nodes

Dimmed nodes

ok, here is some update after my investigation, similar visual dim effect test would be possible, it would look like

test.only("should dim the target Input Data node", async ({ page, palette, nodes, }) => { await palette.dragNewNode({ type: NodeType.BKM, targetPosition: { x: 300, y: 100 }, }); // lines below could be refactored to single fixture method, e.g. `nodes.startDraggingEdge(from, edgeType)` await nodes.select({ name: "Source Node", position: NodePosition.TOP }); await nodes.get({ name: "Source Node" }).getByTitle("Add Information Requirement edge").hover(); await page.mouse.down(); await expect(nodes.get({ name: DefaultNodeName.BKM })).toHaveClass(/.*dimmed/); });

however, was not able to get this working without actual class check, below I put my attempts, that didn't work:

await expect(nodes.get({ name: DefaultNodeName.BKM })).toBeDisabled()

await expect(nodes.get({ name: DefaultNodeName.BKM })).not.toBeEditable()

one more problem is the dim implementation, for some nodes we add dimmed class to existing div:

however for other nodes we add new nested div with dimmed class

Also, I think we could select the node with the this.get() before using the locator(".dimmed"). And I'm not sure if the toBeAttached() is the best option here., we have a toHaveClass assertion, making it unnecessary to add a new assertion method.

jomarko

see my comments inline

jomarko · 2024-03-18T09:58:44Z

packages/dmn-editor/tests/e2e/__fixtures__/nodes.ts

@@ -53,6 +53,10 @@ export enum NodePosition {
 export class Nodes {
  constructor(public page: Page, public diagram: Diagram, public browserName: string) {}

+  public async asserrtIsDimmed(args: { name: string }) {
+    await expect(this.page.locator(".dimmed").locator("span:has-text('" + args.name + "')")).toBeAttached();


this locator was done by me after reading the documentation https://playwright.dev/docs/other-locators#css-matching-by-text

we need to match:

elements with dimmed class, however, we do not know exactly what level of the node structure contain this class - first locator

however we know, inside dimmed there is a text containing node name - second locator

ljmotta · 2024-03-18T12:45:25Z

packages/dmn-editor/tests/e2e/__fixtures__/nodes.ts

+  public async asserrtIsDimmed(args: { name: string }) {
+    await expect(this.page.locator(".dimmed").locator("span:has-text('" + args.name + "')")).toBeAttached();


The correct way to add custom expect is to extend the expect object. You can read more about it here: https://playwright.dev/docs/test-assertions#add-custom-matchers-using-expectextend

Also we have a typo on the method name.

ljmotta · 2024-03-18T12:52:30Z

packages/dmn-editor/tests/e2e/__fixtures__/nodes.ts

@@ -219,4 +220,17 @@ export class Nodes {
        return { x: toBoundingBox.width / 2, y: 10 };
    }
  }
+
+  private getAddTitle(edgeType: EdgeType) {


If we read only the method name we can't say for sure what is getting. Maybe getAddEdgeTitle will give us a better understand of what is happening? WDYT?

yes, totally agree, getAddEdgeTitle brings better clarity of the code, pushing now!

ljmotta · 2024-03-18T12:53:43Z

packages/dmn-editor/tests/e2e/drgRequirements/invalidAuthorityRequirement.spec.ts

+});
+
+test.describe("Invalid edge - Authority Requirement", () => {
+  test.describe("Knowledge Source", () => {


Forgot to change?

yes, thank you for spotting

ljmotta · 2024-03-18T12:59:46Z

packages/dmn-editor/tests/e2e/drgRequirements/invalidInformationRequirement.spec.ts

+    test("shouldn't add an Information Requirement edge from Input Data node to BKM node", async ({
+      palette,
+      edges,
+      nodes,
+    }) => {
+      await palette.dragNewNode({
+        type: NodeType.BKM,
+        targetPosition: { x: 300, y: 100 },
+      });
+
+      await nodes.dragNewConnectedEdge({
+        type: EdgeType.INFORMATION_REQUIREMENT,
+        from: "Source Node",
+        to: DefaultNodeName.BKM,
+      });
+
+      expect(await edges.get({ from: "Source Node", to: DefaultNodeName.BKM })).not.toBeAttached();
+    });


Also, I think we could select the node with the this.get() before using the locator(".dimmed"). And I'm not sure if the toBeAttached() is the best option here., we have a toHaveClass assertion, making it unnecessary to add a new assertion method.

jomarko · 2024-03-19T11:50:17Z

packages/dmn-editor/src/diagram/nodes/Nodes.tsx

@@ -239,7 +239,7 @@ export const InputDataNode = React.memo(
          onDoubleClick={triggerEditing}
          onKeyDown={triggerEditingIfEnter}
          style={alternativeSvgStyle}
-          className={`kie-dmn-editor--input-data-node ${selectedAlternativeClass}`}
+          className={`kie-dmn-editor--input-data-node ${additionalClasses} ${selectedAlternativeClass}`}


With this change we have code:

<div onDoubleClick={triggerEditing} onKeyDown={triggerEditingIfEnter} style={alternativeSvgStyle} className={`kie-dmn-editor--input-data-node ${additionalClasses} ${selectedAlternativeClass}`} ref={ref} tabIndex={-1} data-nodehref={id} data-nodelabel={inputData["@_name"]} > {/* {`render count: ${renderCount.current}`} <br /> */} <div className={`kie-dmn-editor--node ${additionalClasses}`}> <InfoNodePanel isVisible={!isTargeted && shouldActLikeHovered} /> ....

I know we have currently twice ${additionalClasses} used, I thought it could be enough to keep ${additionalClasses} only in the top level div, however in such scenario, node is disabled, user can not add connection to it, however the text is not 'dimmed', see the screenshot below.

I don't think this is the correct approach, we're adding the additionalClasses just to select the desired div without any real change on the editor. To make this change, we need to remove the other. Taking a look into the DmnEditor.css file I could see why removing the class broke the opacity. The css class was declared as .kie-dmn-editor--node.dimmed. What we could do, is to create a new .kie-dmn-editor--input-data-node.dimmed with the same properties.

It's important to test those changes with the alternative visualization too. To change to the alternative, click on the DRD name, and select the "Alternative" option.

ljmotta · 2024-03-19T15:16:51Z

packages/dmn-editor/src/diagram/nodes/Nodes.tsx

@@ -239,7 +239,7 @@ export const InputDataNode = React.memo(
          onDoubleClick={triggerEditing}
          onKeyDown={triggerEditingIfEnter}
          style={alternativeSvgStyle}
-          className={`kie-dmn-editor--input-data-node ${selectedAlternativeClass}`}
+          className={`kie-dmn-editor--input-data-node ${additionalClasses} ${selectedAlternativeClass}`}


I don't think this is the correct approach, we're adding the additionalClasses just to select the desired div without any real change on the editor. To make this change, we need to remove the other. Taking a look into the DmnEditor.css file I could see why removing the class broke the opacity. The css class was declared as .kie-dmn-editor--node.dimmed. What we could do, is to create a new .kie-dmn-editor--input-data-node.dimmed with the same properties.

It's important to test those changes with the alternative visualization too. To change to the alternative, click on the DRD name, and select the "Alternative" option.

ljmotta · 2024-03-19T15:17:16Z

packages/dmn-editor/tests/e2e/drgRequirements/invalidAuthorityRequirement.spec.ts

+
+      await nodes.startDraggingEdge({ from: "Source Node", edgeType: EdgeType.AUTHORITY_REQUIREMENT });
+
+      await expect(nodes.get({ name: DefaultNodeName.INPUT_DATA })).toHaveClass(/.*dimmed/);


Any particular reason to use RegExp over a string?

yes, toHaveClass compares for equality, so we would need to have toHaveClass("kie-dmn-editor--input-data-node dimmed")

jomarko requested review from ederign and tiagobento as code owners March 12, 2024 15:58

jomarko had a problem deploying to ci March 12, 2024 15:58 — with GitHub Actions Failure

jomarko temporarily deployed to ci March 12, 2024 15:58 — with GitHub Actions Inactive

jomarko commented Mar 12, 2024

View reviewed changes

packages/dmn-editor/tests/e2e/drgRequirements/invalidAuthorityRequirement.spec.ts Outdated Show resolved Hide resolved

jomarko had a problem deploying to ci March 13, 2024 09:23 — with GitHub Actions Failure

jomarko temporarily deployed to ci March 13, 2024 09:23 — with GitHub Actions Inactive

jomarko had a problem deploying to ci March 13, 2024 11:13 — with GitHub Actions Error

jomarko commented Mar 13, 2024

View reviewed changes

jomarko had a problem deploying to ci March 13, 2024 11:44 — with GitHub Actions Failure

jomarko temporarily deployed to ci March 13, 2024 11:44 — with GitHub Actions Inactive

jomarko had a problem deploying to ci March 15, 2024 14:53 — with GitHub Actions Failure

jomarko temporarily deployed to ci March 15, 2024 14:53 — with GitHub Actions Inactive

jomarko added 8 commits March 18, 2024 08:35

initialize spec file

2fa7796

invalid edge tests

9df3867

remove not needed test

28acdf7

drop 'From' from test describe

e4050ca

fix GROUP tests

d6189ff

fix test describe - remove From

fd31cf2

add dim tests

6fe0140

refactor dimmed class assertion

e75b167

jomarko force-pushed the kie-issues-946-part-3-invalidEdges branch from 34b45f8 to e75b167 Compare March 18, 2024 09:51

jomarko had a problem deploying to ci March 18, 2024 09:52 — with GitHub Actions Failure

jomarko temporarily deployed to ci March 18, 2024 09:52 — with GitHub Actions Inactive

jomarko commented Mar 18, 2024

View reviewed changes

jomarko temporarily deployed to ci March 18, 2024 12:15 — with GitHub Actions Inactive

jomarko had a problem deploying to ci March 18, 2024 12:15 — with GitHub Actions Failure

ljmotta reviewed Mar 18, 2024

View reviewed changes

incorporate review feedback

93e6987

jomarko had a problem deploying to ci March 19, 2024 09:27 — with GitHub Actions Failure

jomarko temporarily deployed to ci March 19, 2024 09:27 — with GitHub Actions Inactive

remove custom assertion

f15d490

jomarko had a problem deploying to ci March 19, 2024 11:06 — with GitHub Actions Failure

jomarko temporarily deployed to ci March 19, 2024 11:06 — with GitHub Actions Inactive

jomarko had a problem deploying to ci March 19, 2024 11:06 — with GitHub Actions Error

add dimmed class to the DOM top level lement of the InputData node

963eecd

jomarko temporarily deployed to ci March 19, 2024 11:46 — with GitHub Actions Inactive

jomarko had a problem deploying to ci March 19, 2024 11:46 — with GitHub Actions Failure

jomarko temporarily deployed to ci March 19, 2024 11:46 — with GitHub Actions Inactive

jomarko commented Mar 19, 2024

View reviewed changes

jomarko temporarily deployed to ci March 19, 2024 13:11 — with GitHub Actions Inactive

jomarko had a problem deploying to ci March 19, 2024 13:12 — with GitHub Actions Failure

ljmotta reviewed Mar 19, 2024

View reviewed changes

test alternative input data shape

1e525c6

jomarko temporarily deployed to ci March 19, 2024 16:26 — with GitHub Actions Inactive

jomarko had a problem deploying to ci March 19, 2024 16:26 — with GitHub Actions Failure

ljmotta merged commit ff23ad6 into ljmotta:kie-issues-946-part-3-main Mar 20, 2024
3 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introduce invalid[Edge].spec.ts #24

introduce invalid[Edge].spec.ts #24

jomarko commented Mar 12, 2024

jomarko Mar 13, 2024 •

edited

Loading

jomarko Mar 13, 2024

ljmotta Mar 18, 2024

jomarko left a comment

jomarko Mar 18, 2024

ljmotta Mar 18, 2024

ljmotta Mar 18, 2024

jomarko Mar 19, 2024

ljmotta Mar 18, 2024

jomarko Mar 19, 2024

ljmotta Mar 18, 2024

jomarko Mar 19, 2024

ljmotta Mar 19, 2024

ljmotta Mar 19, 2024

ljmotta Mar 19, 2024

jomarko Mar 19, 2024

		public async asserrtIsDimmed(args: { name: string }) {
		await expect(this.page.locator(".dimmed").locator("span:has-text('" + args.name + "')")).toBeAttached();


		await nodes.startDraggingEdge({ from: "Source Node", edgeType: EdgeType.AUTHORITY_REQUIREMENT });

		await expect(nodes.get({ name: DefaultNodeName.INPUT_DATA })).toHaveClass(/.*dimmed/);

introduce invalid[Edge].spec.ts #24

introduce invalid[Edge].spec.ts #24

Conversation

jomarko commented Mar 12, 2024

jomarko Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Non dimmed nodes

Dimmed nodes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jomarko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jomarko Mar 13, 2024 •

edited

Loading