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

Test to confirm correct UI and cache state after frame deletion #8949

Merged
merged 15 commits into from
Jan 23, 2025

Conversation

archibald1418
Copy link
Contributor

@archibald1418 archibald1418 commented Jan 15, 2025

Motivation and context

Fix #8872 dealt with incorrect states of UI and cache after frame deletion. The frame was not being deleted and the cache was not updated properly to reflect the changes.

How has this been tested?

Set up

  • open Main task job
  • save default cvat.config.jobMetaDataReloadPeriod value from cvat-core/src/config.ts
  • temporarily set jobMetaDataReloadPeriod to a small value (100 ms) for the sake of easier testing

Test suite

  • Inside the job, wait for the small job metadata reload period to elapse
  • Transition to the next frame by clicking the next button on the player
  • Verify that GET /data/meta is sent and that the response has frames in it.
  • Save current frame number to the variable frameNum
  • Delete the frame: click on the bin, click on 'Delete' button in the modal
  • Ensure that 'Restore frame' button is absent
  • Make sure that the frame is deleted by asserting that current frame has number frameNum + 1 (the next frame in the dataset)
  • Click Save to send PATCH to /data/meta.
  • Intercept the request to validate that response has deleted_frames property, that it's an Array and that it contains the deleted frame number frameNum
  • Assert that deleted_frames array contains one element: frameNum

Tear down

  • Set jobMetaDataReloadPeriod back to the saved default value

Checklist

  • I submit my changes into the develop branch
  • I have created a changelog fragment
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • I have linked related issues (see GitHub docs)
  • I have increased versions of npm packages if it is necessary
    (cvat-canvas,
    cvat-core,
    cvat-data and
    cvat-ui)

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.

@archibald1418 archibald1418 requested a review from klakhov January 15, 2025 20:57
@archibald1418 archibald1418 changed the title Test to confirm correct frame deletion after fix #8872 Test to confirm correct UI and cache state after frame deletion Jan 15, 2025
@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.90%. Comparing base (c6ec4eb) to head (dbae90e).
Report is 2 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #8949      +/-   ##
===========================================
+ Coverage    73.83%   73.90%   +0.06%     
===========================================
  Files          417      417              
  Lines        44602    44598       -4     
  Branches      4031     4031              
===========================================
+ Hits         32934    32958      +24     
+ Misses       11668    11640      -28     
Components Coverage Δ
cvat-ui 78.39% <ø> (+0.11%) ⬆️
cvat-server 70.08% <67.97%> (+0.01%) ⬆️

@archibald1418 archibald1418 requested a review from klakhov January 20, 2025 10:22
tests/cypress/support/utils_e2e.js Outdated Show resolved Hide resolved
tests/cypress/support/commands.js Outdated Show resolved Hide resolved
tests/cypress/e2e/issues_prs2/issue_8872_delete_frame.js Outdated Show resolved Hide resolved
tests/cypress/e2e/issues_prs2/issue_8872_delete_frame.js Outdated Show resolved Hide resolved
Copy link
Contributor

@klakhov klakhov left a comment

Choose a reason for hiding this comment

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

It seems we wont be using year in headers anymore.

LGTM

tests/cypress/e2e/issues_prs2/issue_8872_delete_frame.js Outdated Show resolved Hide resolved
});
});

Cypress.Commands.add('clickSave', (force = false) => {
Copy link
Member

@bsekachev bsekachev Jan 22, 2025

Choose a reason for hiding this comment

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

I will suggest being more specific here in naming if we add global Cypress command.
We probably have many Save buttons across the application.

So, maybe clickSaveAnnotationView.
Additionally I will ask to search by contains('Save').click over the tests and replace findings

I found at least two more places with this code (case 92, case 91):

cy.get('button').contains('Save').click();
cy.get('button').contains('Save').trigger('mouseout');

Copy link
Member

@bsekachev bsekachev left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@archibald1418 archibald1418 merged commit 4191be3 into develop Jan 23, 2025
34 checks passed
@archibald1418 archibald1418 deleted the ov/test-fix-delete-frame-issue branch January 23, 2025 16:32
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.

4 participants