#2238 Write function to compare transform string in Cypress tests #2225

Jerinjk14 · 2024-10-28T13:29:28Z

Fixes: #2238
Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

Signed-off-by: Jerin George <[email protected]>

…ng comments Signed-off-by: Jerin George <[email protected]>

tomlyn · 2024-11-05T17:59:38Z

canvas_modules/harness/cypress/e2e/canvas/zoom.cy.js

 		// Test get Zoom to reveal & ZoomTo
 		cy.selectEntryFromDropdown("Histogram");
 		cy.setXPercentOffset(70);
 		cy.setYPercentOffset(60);
 		cy.submitAPI();
+		cy.wait(500);


Are these waits necessary? It seems strange that you are adding them even in places where the code was running OK before. We should remove all unnecessary waits. Also, if we do have to add them 500 (half a second) seems like a long time to wait. While it won't affect the overall run time of the test too much by adding them it is our usual practice to keep them as small as reasonably possible.

After api calls there needs a minimum wait time required, Test will fail if the wait time less than 500ms as it required after api call.

I'm not sure how you can say that since the verify code isn't / wasn't working correctly. Do you mean these are needed when the test run on your own Mac ?

tomlyn · 2024-11-05T18:04:42Z

canvas_modules/harness/cypress/e2e/canvas/zoom.cy.js

-		// TODO -- write function to test these +/- 2 px.
-		// cy.verifyCanvasTransform("translate(498.32727272727266,290.85454545454536) scale(0.9090909090909091)");
+		cy.wait(500);
+		cy.verifyCanvasTransform("translate(498.32727272727266,290.85454545454536) scale(0.9090909090909091)");


Did you test to see if your code will return an error if an incorrect value is provided? I tried changing this line to:

cy.verifyCanvasTransform("translate(200,290.85454545454536) scale(0.9090909090909091)");

That is, with 200 as the x parameter and the test passed!

When writing any tests, it is just as important to make sure the test will fail, in an invalid situation, as it is to make sure the test passes. If the test doesn't fail in the invalid situation then there would be no reason to run the test in the first place -- right?

I can see this error is occurring due to logic of checking the variable type, whether its a string or not, changed with
if (typeof movString === "string") ,
now if the pixel range is other than +/-2 px, the test will fail

tomlyn · 2024-11-05T18:06:48Z

canvas_modules/harness/cypress/support/canvas/verification-cmds.js

-		.should("eq", movString);
+	// Verify the argument passed is a string
+	if (movString === String) {
+		cy.get("#canvas-div-0 .d3-canvas-group", { timeout: 10000 })


Is the timeout needed here. I see from the docs that the default is 4000 (4 seconds) which should be ample time for anything to happen in Common Canvas. We don't set this timeout anywhere else in the Cypress tests so if it isn't needed please remove it.

tomlyn · 2024-11-05T18:09:22Z

canvas_modules/harness/cypress/support/canvas/verification-cmds.js

+				compareCloseTo(actualValues.translateY, expectedValues.translateY);
+				compareCloseTo(actualValues.scale, expectedValues.scale);
+			});
+	}


I know it ought to be unlikely but, what happens here if movString is not a string? Will the function return as if the comparison succeeded? If so that not what we want as we want the test to fail in all error situations.

If you are testing for a string higher up in the function because sometimes zoom.cy.js passes in an undefined value then your code should specifically test for "undefined" and return success and in any other case (other than "string" ) return failure. In fact, since undefined is a value that is specifically passed in by zoom.cy.js, what the function ought to do is make sure the actualTransformString is undefined in that situation.

I am able to fix this with if,else condition to check whether the movString is undefined, also if movString is not a string the test will fail with below error,

Although I'm sure it works, that's a strange way to write the if / else if / else code. It would read better if you have the two "positive" cases first and then a catch all case which returns the error in the bottom (as the else).

tomlyn · 2024-11-05T18:18:24Z

canvas_modules/harness/cypress/support/canvas/verification-cmds.js

-		.invoke("attr", "transform")
-		.should("eq", movString);
+	// Verify the argument passed is a string
+	if (movString === String) {


This isn't how you test for a string. This line of code is the reason invalid test situations are returning success (as mentioned in my comment above). You should use typeof to test for the type of a variable.

tomlyn · 2024-11-06T18:17:15Z

canvas_modules/harness/cypress/support/canvas/verification-cmds.js

+				compareCloseTo(actualValues.translateY, expectedValues.translateY);
+				compareCloseTo(actualValues.scale, expectedValues.scale);
+			});
+	}


Although I'm sure it works, that's a strange way to write the if / else if / else code. It would read better if you have the two "positive" cases first and then a catch all case which returns the error in the bottom (as the else).

tomlyn · 2024-11-06T18:19:17Z

canvas_modules/harness/cypress/e2e/canvas/zoom.cy.js

 		// Test get Zoom to reveal & ZoomTo
 		cy.selectEntryFromDropdown("Histogram");
 		cy.setXPercentOffset(70);
 		cy.setYPercentOffset(60);
 		cy.submitAPI();
+		cy.wait(500);


I'm not sure how you can say that since the verify code isn't / wasn't working correctly. Do you mean these are needed when the test run on your own Mac ?

…ests (elyra-ai#2225) Signed-off-by: Jerin George <[email protected]>

Jerinjk14 · 2024-11-08T15:09:07Z

@tomlyn ,I have pushed new commit as the issue with the cy.wait is because of using .then instead of .should, we can ignore the unnecessary wait time now.

tomlyn

LGTM

Jerinjk14 added 2 commits October 28, 2024 17:47

#3886 Write function to compare transform string in Cypress tests

52e7747

Signed-off-by: Jerin George <[email protected]>

#3886 Write function to compare transform string in Cypress tests/fixing

0c00882

Signed-off-by: Jerin George <[email protected]>

Jerinjk14 closed this Nov 5, 2024

Jerinjk14 reopened this Nov 5, 2024

Jerinjk14 added 2 commits November 5, 2024 20:54

3886 Write function to compare transform string in Cypress tests-addi…

344b96a

…ng comments Signed-off-by: Jerin George <[email protected]>

Merge branch 'main' into cypress-functionForStringCompare

3c72b82

Jerinjk14 requested a review from tomlyn November 5, 2024 15:50

tomlyn requested changes Nov 5, 2024

View reviewed changes

tomlyn requested changes Nov 6, 2024

View reviewed changes

Jerinjk14 changed the title ~~#3886 Write function to compare transform string in Cypress tests/fixing~~ #2238 Write function to compare transform string in Cypress tests Nov 7, 2024

Jerinjk14 added 2 commits November 8, 2024 13:35

Merge branch 'main' into cypress-functionForStringCompare

8e0531d

elyra-ai#2238 Write function to compare transform string in Cypress t…

2ee75bc

…ests (elyra-ai#2225) Signed-off-by: Jerin George <[email protected]>

tomlyn approved these changes Nov 8, 2024

View reviewed changes

tomlyn merged commit 32f2a74 into elyra-ai:main Nov 8, 2024
3 checks passed

Jerinjk14 deleted the cypress-functionForStringCompare branch November 12, 2024 07:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2238 Write function to compare transform string in Cypress tests #2225

#2238 Write function to compare transform string in Cypress tests #2225

Jerinjk14 commented Oct 28, 2024 •

edited

Loading

tomlyn Nov 5, 2024

Jerinjk14 Nov 6, 2024

tomlyn Nov 6, 2024

tomlyn Nov 5, 2024

Jerinjk14 Nov 6, 2024

tomlyn Nov 5, 2024

tomlyn Nov 5, 2024

Jerinjk14 Nov 6, 2024

tomlyn Nov 6, 2024

tomlyn Nov 5, 2024

tomlyn Nov 6, 2024

tomlyn Nov 6, 2024

Jerinjk14 commented Nov 8, 2024

tomlyn left a comment

#2238 Write function to compare transform string in Cypress tests #2225

#2238 Write function to compare transform string in Cypress tests #2225

Conversation

Jerinjk14 commented Oct 28, 2024 • edited Loading

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

Jerinjk14 commented Nov 8, 2024

tomlyn left a comment

Choose a reason for hiding this comment

Jerinjk14 commented Oct 28, 2024 •

edited

Loading