Automation for Creating Polygons from Existing Points #2360

nstclair-cc · 2024-07-29T18:18:16Z

Pull Request: Automation for Creating Polygons from Existing Points

Pivotal Tracker Story: #187809206

Description:
This PR introduces Cypress tests for automating the process of creating polygons from existing points. The tests include the following steps:

Add points to the table.
Click the "Graph IT!" button to create the polygon.
Wait for the Geometry tile to appear.

Questions:

Do these tests fit better in geometry.spec.ts or one of the table tile tests?
Do we want to add anything, such as resizing a polygon with the keyboard keys and confirming the resize functionality is working?

I have put in the PR to avoid further rebase issues, but please don't hesitate to ask any questions. Thank you!

Changes Included:

Added Cypress tests for creating polygons from existing points.
Tests include adding points to the table, clicking the "Graph IT!" button, and waiting for the Geometry tile to appear.

codecov · 2024-07-29T18:19:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.64%. Comparing base (72fee9a) to head (7316e78).
Report is 24 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (72fee9a) and HEAD (7316e78). Click for more details.

HEAD has 40 uploads less than BASE

Flag BASE (72fee9a) HEAD (7316e78)

cypress-regression 5 0

cypress 35 0

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #2360       +/-   ##
===========================================
- Coverage   85.85%   54.64%   -31.21%     
===========================================
  Files         738      729        -9     
  Lines       37855    37704      -151     
  Branches     9635     9612       -23     
===========================================
- Hits        32502    20605    -11897     
- Misses       5046    16072    +11026     
- Partials      307     1027      +720

Flag	Coverage Δ
cypress	`?`
cypress-regression	`?`
cypress-smoke	`28.07% <ø> (+<0.01%)`	⬆️
jest	`48.73% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cypress · 2024-07-29T22:32:05Z

collaborative-learning Run #13685

Run Properties: Passed #13685 • 7316e7870a: Merge branch '187809206-automation-polygon-points' of https://github.com/concord...

Project	`collaborative-learning`
Branch Review	`187809206-automation-polygon-points`
Run status	`Passed #13685`
Run duration	`01m 44s`
Commit	`7316e7870a: Merge branch '187809206-automation-polygon-points' of https://github.com/concord...`
Committer	`Boris Goldowsky`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`0`
Skipped	`0`
Passing	`1`
View all changes introduced in this branch ↗︎

bgoldowsky

Do these tests fit better in geometry.spec.ts or one of the table tile tests?

The test you have written most properly belongs in geometry_table_integration_test_spec.js, since it involves both geometry and table tiles.

Do we want to add anything, such as resizing a polygon with the keyboard keys and confirming the resize functionality is working?

A test of keyboard functions wouldn't hurt - there isn't really a resize, but the arrow keys should work to move a point (or several points, whatever is selected). Also the delete and backspace keys should work to remove objects.

As far as testing the creation of polygons from existing points - I would take a look at geometry_tool_spec.js lines 280-292 which has the most basic version of this, creating a triangle from three points. What I think would be most useful is to extend this to capture some of the related functionality:

Create a second polygon that shares a point with the first polygon.
After creating a polygon, click one of its vertexes to go back into editing it, add a point or two, then close it again. Try adding both a new point and an existing point.

nstclair-cc · 2024-07-30T22:02:13Z

Thanks, @bgoldowsky! I'll take a crack at it tomorrow. Those fixes make sense to me!

…lygon

…points, Add a point to the existing polygon

nstclair-cc · 2024-08-02T18:59:31Z

@bgoldowsky - thank you for your feedback! I made the changes you requested, and I believe we now have coverage for what we need. However, please let me know if any additional tests should be implemented.

One thing I noticed: I added a test for "Delete point using keyboard command," but I saw that the subsequent test (labeling points) now looks for labels "BCA" instead of "ABC." Is this a bug? It doesn't seem to cause issues in the e2e specs, but it might impact undo/redo in the DOM, for example. Let me know what you think—I wasn't entirely sure what’s happening there.

Also, at the end of the circle tests, I noticed two trailing points that don’t get deleted. Is that expected behavior? Just wanted to double-check.

For the delete points test, I used undo to revert the deleted point and start fresh for the next test. I'm wondering if that's best practice since Cypress has a spec below mine that explicitly tests undo/redo.

bgoldowsky

This is great progress! - there are a few issues that I have noted that still need attention though.

To answer your questions:

The change from "ABC" to "BCA" is a little mysterious. I think it may have to do with deleting one of the points and then re-creating it, the order gets altered.
The circle test deletes a point at the end but doesn't delete all of the points - I think it's fine. It is just making sure that the circle goes away if either of the points that define it is removed.
I think using 'undo' to reset things after a test is ok, but it can make it a little harder to read and reason about the test since exactly how much gets reverted by clicking undo once is not always obvious.

bgoldowsky · 2024-08-05T19:39:47Z

cypress/e2e/functional/tile_tests/geometry_tool_spec.js

+    // Verify that the point has moved to approximately (5.1, 4.9)
+    geometryToolTile.getGraphPoint().then(($points) => {
+        const movedPoint = $points.filter((index, el) => {
+            const cx = parseFloat(el.getAttribute('cx'));
+            const cy = parseFloat(el.getAttribute('cy'));
+            return Math.abs(cx - 5.1) < 0.1 && Math.abs(cy - 4.9) < 0.1;
+        });
+        expect(movedPoint).to.have.length(0);


I think you meant to write expect(movedPoint).to.have.length(1), rather than 0 here, to test that a point matching that description does exist. Unfortunately I don't think this can work since the cx and cy attributes in the SVG are in screen pixel units, not the units that are shown on the graph axis.

What I would suggest is to turn on the coordinates label for the point before moving it -- so if it's selected, something like

clueCanvas.clickToolbarButton('geometry', 'label'); geometryToolTile.chooseLabelOption('length'); geometryToolTile.getGraphPointLabel().contains('5.00, 5.00').should('exist');

Then move it and test that the label should show the new coordinates.

And/or, check that the angle label, which was 90 degrees in line 213, shows some other value.

Just to add to Boris's idea here: rather than just checking that the cx & cy points, or the angle, have changed, you could check that the values are greater than (for the cx value) or less than (for the cy value) the previous values. (I'm not sure which angle is referred to here, or whether that angle would increase or decrease, but the same idea applies.)

cypress/e2e/functional/tile_tests/geometry_tool_spec.js

bgoldowsky · 2024-08-05T19:51:29Z

cypress/e2e/functional/tile_tests/geometry_tool_spec.js

+    // Verify that the point has moved to approximately (15.1, 4.9)
+    geometryToolTile.getGraphPoint().then(($points) => {
+        const movedPoint = $points.filter((index, el) => {
+            const cx = parseFloat(el.getAttribute('cx'));
+            const cy = parseFloat(el.getAttribute('cy'));
+            return Math.abs(cx - 15.1) < 0.1 && Math.abs(cy - 4.9) < 0.1;
+        });
+        expect(movedPoint).to.have.length(0);
+    });


Ideally we'd like some way to check that both polygons have been modified by moving the single shared point. Perhaps a segment in each polygon could be labeled with its length, and you could check that both lengths were altered?

cypress/e2e/functional/tile_tests/geometry_tool_spec.js

cypress/e2e/functional/tile_tests/geometry_table_integraton_test_spec.js

johnTcrawford

I have little to add to Boris's comments - just that one small comment I made. Aside from those suggestions, it looks good to me: I Approve, with those changes.

johnTcrawford · 2024-08-12T15:31:38Z

cypress/e2e/functional/tile_tests/geometry_tool_spec.js

+    // Verify that the point has moved to approximately (5.1, 4.9)
+    geometryToolTile.getGraphPoint().then(($points) => {
+        const movedPoint = $points.filter((index, el) => {
+            const cx = parseFloat(el.getAttribute('cx'));
+            const cy = parseFloat(el.getAttribute('cy'));
+            return Math.abs(cx - 5.1) < 0.1 && Math.abs(cy - 4.9) < 0.1;
+        });
+        expect(movedPoint).to.have.length(0);


Just to add to Boris's idea here: rather than just checking that the cx & cy points, or the angle, have changed, you could check that the values are greater than (for the cx value) or less than (for the cy value) the previous values. (I'm not sure which angle is referred to here, or whether that angle would increase or decrease, but the same idea applies.)

…n-polygon-points

…ed point

nstclair-cc · 2024-09-06T16:23:08Z

In this PR, I made the following changes:

Refined the checks related to graph point movement with keyboard strokes (following John's feedback).
Removed the XYTile checks.
Added a check for polygon length when points are shared.
Improved the checks to ensure polygon lengths are as expected in the polygon with shared point checks.

For the labeling issue, I moved the test spec farther up in the sequence to prevent labels from shifting around. Once this code is merged, I'll do a quick sniff test for bugs (see Pivotal Tracker story).

I also noticed that in the Cypress test with shared points, the angle measurement seems to fluctuate. While I addressed this in the code, I’m considering removing this check, as the labels on the line segments seem to work well. I’m wondering if Cypress may have caught a bug here, so I’ll do some manual testing to determine if it’s a Cypress quirk or something more.

johnTcrawford

Looks good, I Approve - i.e., I don't need to review this again. However, I made several comments, of which the one about:
expect(angleText).not.to.equal('90...
(my second comment below) should at least be looked at, and changed if I am correct about it.

cypress/e2e/functional/tile_tests/geometry_tool_spec.js

johnTcrawford · 2024-09-06T18:05:58Z

cypress/e2e/functional/tile_tests/geometry_tool_spec.js

+    // Also check that the angle label has changed from its original value
+    geometryToolTile.getAngleAdornment().should(($label) => {
+        const angleText = $label.text();
+        expect(angleText).not.to.equal('90'); // 90° was the original value


Shouldn't this be:
expect(angleText).not.to.equal('90°');
including the degree sign (°), or am I missing something? Alternatively, you could use ".not.contain('90')".

Does not appear that this was resolved.
The matching test before the point was moved (line 268) is
geometryToolTile.getGraphPointLabel().contains('90°').should('exist');
So this should be the opposite of that, eg
expect(angleText).not.contains('90°');

I went ahead and checked in the fix since it was tiny & I wanted to test it anyway.

cypress/e2e/functional/tile_tests/geometry_tool_spec.js

johnTcrawford · 2024-09-06T18:45:57Z

cypress/e2e/functional/tile_tests/geometry_tool_spec.js

+    // Verify that the angle has decreased by 1 degree
+    cy.get('@initialAngle').then((initialAngle) => {
+      const newAngle = initialAngle - 1;
+      geometryToolTile.getGraphPointLabel().contains(`${newAngle}°`).should('exist'); // Check if angle decreased by 1 degree


As above for cx & cy, you could change this to check that the value is less, rather than a specific value; however, if you do, you would need to parse out (i.e. remove) the degree sign (°) from the new value returned by getGraphPointLabel(), so that you can compare integers.

…com/concord-consortium/collaborative-learning into 187809206-automation-polygon-points

bgoldowsky

This looks great! 👍

bgoldowsky · 2024-09-10T11:28:16Z

cypress/e2e/functional/tile_tests/geometry_tool_spec.js

+    // Also check that the angle label has changed from its original value
+    geometryToolTile.getAngleAdornment().should(($label) => {
+        const angleText = $label.text();
+        expect(angleText).not.to.equal('90'); // 90° was the original value


Does not appear that this was resolved.
The matching test before the point was moved (line 268) is
geometryToolTile.getGraphPointLabel().contains('90°').should('exist');
So this should be the opposite of that, eg
expect(angleText).not.contains('90°');

I went ahead and checked in the fix since it was tiny & I wanted to test it anyway.

nstclair-cc requested review from bgoldowsky and johnTcrawford July 29, 2024 18:18

nstclair-cc changed the title ~~Rebase issuesAutomation for Creating Polygons from Existing Points~~ Automation for Creating Polygons from Existing Points Jul 29, 2024

nstclair-cc temporarily deployed to branches July 29, 2024 22:31 — with GitHub Actions Inactive

nstclair-cc marked this pull request as ready for review July 29, 2024 22:32

bgoldowsky requested changes Jul 30, 2024

View reviewed changes

nstclair-cc force-pushed the 187809206-automation-polygon-points branch from d10fc0f to fde91f7 Compare July 31, 2024 18:05

nstclair-cc temporarily deployed to branches July 31, 2024 18:09 — with GitHub Actions Inactive

nstclair-cc force-pushed the 187809206-automation-polygon-points branch from fde91f7 to d83ab5e Compare August 1, 2024 23:36

nstclair-cc temporarily deployed to branches August 1, 2024 23:40 — with GitHub Actions Inactive

nstclair-cc added 7 commits August 2, 2024 10:08

Rebase issues

85968d6

moved tests to geometry_table_test.spec.js

42f417e

remove spacing

fd3abc4

added tests for keyboard commands with polygon adjustments

a386349

added delete a point test

6e6911e

outlined creation of second polygon that shares a point with first po…

ff11625

…lygon

Add tests for keyboard functions, Create first polygon from existing …

f90f5a1

…points, Add a point to the existing polygon

nstclair-cc force-pushed the 187809206-automation-polygon-points branch from d83ab5e to f90f5a1 Compare August 2, 2024 18:52

nstclair-cc temporarily deployed to branches August 2, 2024 18:56 — with GitHub Actions Inactive

deleted arbitrary log

b5ab86d

nstclair-cc requested a review from bgoldowsky August 2, 2024 19:03

nstclair-cc temporarily deployed to branches August 2, 2024 19:07 — with GitHub Actions Inactive

bgoldowsky requested changes Aug 5, 2024

View reviewed changes

johnTcrawford reviewed Aug 13, 2024

View reviewed changes

nstclair-cc added 2 commits August 22, 2024 14:53

Merge remote-tracking branch 'origin/master' into 187809206-automatio…

274fd2d

…n-polygon-points

incorperated feedback in PR 2360

074977f

nstclair-cc added 4 commits September 5, 2024 16:18

Check that both polygons have been modified by moving the single shar…

170d439

…ed point

Clean up polygon label test

bd94bc1

added a check for polygon length with shared points

792a700

Removed arbitrary logs

6c5a52f

nstclair-cc requested a review from johnTcrawford September 6, 2024 00:47

nstclair-cc temporarily deployed to branches September 6, 2024 00:51 — with GitHub Actions Inactive

nstclair-cc added 2 commits September 5, 2024 17:59

removed XY Plot specs

a131525

Cleaned up code

4a2f972

nstclair-cc temporarily deployed to branches September 6, 2024 01:05 — with GitHub Actions Inactive

nstclair-cc temporarily deployed to branches September 6, 2024 16:21 — with GitHub Actions Inactive

johnTcrawford approved these changes Sep 6, 2024

View reviewed changes

johnTcrawford reviewed Sep 6, 2024

View reviewed changes

Incorporated PR feedback

25742dc

nstclair-cc force-pushed the 187809206-automation-polygon-points branch from de63367 to 25742dc Compare September 6, 2024 23:10

nstclair-cc temporarily deployed to branches September 6, 2024 23:14 — with GitHub Actions Inactive

Clean up lint error

141d618

nstclair-cc requested a review from bgoldowsky September 9, 2024 17:12

nstclair-cc temporarily deployed to branches September 9, 2024 17:15 — with GitHub Actions Inactive

Merge branch 'master' into 187809206-automation-polygon-points

adad6ac

nstclair-cc temporarily deployed to branches September 9, 2024 23:43 — with GitHub Actions Inactive

bgoldowsky added 2 commits September 10, 2024 07:36

Small consistency fix.

6c39d67

Merge branch '187809206-automation-polygon-points' of https://github.…

7316e78

…com/concord-consortium/collaborative-learning into 187809206-automation-polygon-points

bgoldowsky approved these changes Sep 10, 2024

View reviewed changes

bgoldowsky temporarily deployed to branches September 10, 2024 11:40 — with GitHub Actions Inactive

nstclair-cc merged commit 2a45ef4 into master Sep 10, 2024
11 of 12 checks passed

nstclair-cc deleted the 187809206-automation-polygon-points branch September 10, 2024 21:59

nstclair-cc restored the 187809206-automation-polygon-points branch September 10, 2024 22:34

nstclair-cc mentioned this pull request Sep 10, 2024

Revert "Automation for Creating Polygons from Existing Points" #2400

Merged

nstclair-cc temporarily deployed to branches September 10, 2024 22:38 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automation for Creating Polygons from Existing Points #2360

Automation for Creating Polygons from Existing Points #2360

nstclair-cc commented Jul 29, 2024 •

edited

Loading

codecov bot commented Jul 29, 2024 •

edited

Loading

cypress bot commented Jul 29, 2024 •

edited

Loading

bgoldowsky left a comment

nstclair-cc commented Jul 30, 2024

nstclair-cc commented Aug 2, 2024

bgoldowsky left a comment

bgoldowsky Aug 5, 2024

johnTcrawford Aug 12, 2024 •

edited

Loading

bgoldowsky Aug 5, 2024

johnTcrawford left a comment •

edited

Loading

johnTcrawford Aug 12, 2024 •

edited

Loading

nstclair-cc commented Sep 6, 2024

johnTcrawford left a comment •

edited

Loading

johnTcrawford Sep 6, 2024

bgoldowsky Sep 10, 2024 •

edited

Loading

johnTcrawford Sep 6, 2024

bgoldowsky left a comment

bgoldowsky Sep 10, 2024 •

edited

Loading

Automation for Creating Polygons from Existing Points #2360

Automation for Creating Polygons from Existing Points #2360

Conversation

nstclair-cc commented Jul 29, 2024 • edited Loading

Pull Request: Automation for Creating Polygons from Existing Points

codecov bot commented Jul 29, 2024 • edited Loading

Codecov Report

cypress bot commented Jul 29, 2024 • edited Loading

collaborative-learning Run #13685

bgoldowsky left a comment

Choose a reason for hiding this comment

nstclair-cc commented Jul 30, 2024

nstclair-cc commented Aug 2, 2024

bgoldowsky left a comment

Choose a reason for hiding this comment

bgoldowsky Aug 5, 2024

Choose a reason for hiding this comment

johnTcrawford Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

bgoldowsky Aug 5, 2024

Choose a reason for hiding this comment

johnTcrawford left a comment • edited Loading

Choose a reason for hiding this comment

johnTcrawford Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

nstclair-cc commented Sep 6, 2024

johnTcrawford left a comment • edited Loading

Choose a reason for hiding this comment

johnTcrawford Sep 6, 2024

Choose a reason for hiding this comment

bgoldowsky Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

johnTcrawford Sep 6, 2024

Choose a reason for hiding this comment

bgoldowsky left a comment

Choose a reason for hiding this comment

bgoldowsky Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

nstclair-cc commented Jul 29, 2024 •

edited

Loading

codecov bot commented Jul 29, 2024 •

edited

Loading

cypress bot commented Jul 29, 2024 •

edited

Loading

johnTcrawford Aug 12, 2024 •

edited

Loading

johnTcrawford left a comment •

edited

Loading

johnTcrawford Aug 12, 2024 •

edited

Loading

johnTcrawford left a comment •

edited

Loading

bgoldowsky Sep 10, 2024 •

edited

Loading

bgoldowsky Sep 10, 2024 •

edited

Loading