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

Updating tests for migration to a new version of antd #3623

Merged
merged 69 commits into from
Sep 13, 2021

Conversation

dvkruchinin
Copy link
Contributor

Depends on #3558

Motivation and context

Additional test update.

How has this been tested?

Checklist

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.
  • I have updated the license header for each file (see an example below)
# Copyright (C) 2021 Intel Corporation
#
# SPDX-License-Identifier: MIT

@dvkruchinin dvkruchinin changed the title [WIP] Updating tests for migration to a new version of antd Updating tests for migration to a new version of antd Sep 2, 2021
@dvkruchinin
Copy link
Contributor Author

@bsekachev
PR is ready for review.
As it turned out, the problem turned out to be in the procedure for closing the popover. When drawing fast identical shapes, cypress could turn to the popover which is currently still closing. Which led to an error.
I have revert the last changes in file handle-popover-visibility.tsx
The adaptation and verification of passing tests in the Firefox browser was also carried out: https://github.com/dvkruchinin/cvat/actions/runs/1193031673

@dvkruchinin dvkruchinin changed the base branch from bs/migration to develop September 2, 2021 11:34
@@ -23,19 +23,15 @@ export default function withVisibilityHandling(WrappedComponent: typeof Popover,
{...rest}
trigger={visible ? 'click' : 'hover'}
overlayClassName={overlayClassNames.join(' ').trim()}
afterVisibleChange={(_visible: boolean) => {
Copy link
Member

Choose a reason for hiding this comment

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

We use afterVisibleChange, but it is not a part of antd API, so it can be changed in the future.
What is more, as I found, afterVisibleChange does not guarantee that the popover does not have pointer-events: none and it is triggered several times. .
So, maybe current implementation with (waitFor) is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Returned.

@@ -242,7 +242,11 @@ Cypress.Commands.add('openTaskJob', (taskName, jobID = 0, removeAnnotations = tr

Cypress.Commands.add('interactControlButton', (objectType) => {
cy.get('body').focus();
cy.get(`.cvat-${objectType}-control`).trigger('mouseleave').trigger('mouseout').trigger('mouseover');
cy.get(`.cvat-${objectType}-control`).trigger('mouseleave').trigger('mouseout').trigger('mousemove').trigger('mouseover');
cy.get(`.cvat-${objectType}-control`).should('have.class', 'ant-popover-open');
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid using ant-* classes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought to use this as an additional check. But it works without it. Deleted it.

@@ -33,6 +41,9 @@ Cypress.Commands.add('opencvCreateShape', (opencvShapeParams) => {
});

Cypress.Commands.add('opncvCheckObjectParameters', (objectType) => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Cypress.Commands.add('opncvCheckObjectParameters', (objectType) => {
Cypress.Commands.add('opencvCheckObjectParameters', (objectType) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment on lines +9 to +17
Cypress.Commands.add('interactOpenCVControlButton', () => {
cy.get('body').focus();
cy.get('.cvat-tools-control').trigger('mouseleave').trigger('mouseout').trigger('mousemove').trigger('mouseover');
cy.get('.cvat-tools-control').should('have.class', 'ant-popover-open');
cy.get('.cvat-opencv-control-popover-visible').should('exist');
cy.get('.cvat-opencv-control-popover-visible').should('be.visible');
cy.get('.cvat-opencv-control-popover-visible').should('have.attr', 'style').and('not.include', 'pointer-events');
});

Copy link
Member

Choose a reason for hiding this comment

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

Why not to use cy.interactControlButton?

Copy link
Contributor Author

@dvkruchinin dvkruchinin Sep 2, 2021

Choose a reason for hiding this comment

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

In this case, need to either heavily rework the command, or rename the classes. I think it's easier to create a separate command for this.

Comment on lines 284 to 286
cy.get(`.cvat-draw-${objectType.toLowerCase()}-popover-visible`).should('not.exist');
cy.get(`.cvat-draw-${objectType.toLowerCase()}-popover`).should('be.hidden');
cy.get(`.cvat-draw-${objectType.toLowerCase()}-popover`).should('have.attr', 'style').and('include', 'pointer-events: none');
Copy link
Member

Choose a reason for hiding this comment

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

These actions are not expected to be in command with name checkObjectParameters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved it to a separate command.

Comment on lines +21 to +22
cy.get('.cvat-draw-cuboid-popover-visible').should('not.exist');
cy.get('.cvat-draw-cuboid-popover').should('be.hidden');
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure we should test that the popovers are closed now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this check, there is a high probability of getting error AssertionError: Timed out retrying after 25000ms: Expected to find element: `.*-popover-visible`, but never found it.

Comment on lines 122 to 124
cy.get('.cvat-opencv-control-popover-visible').should('not.exist');
cy.get('.cvat-opencv-control-popover').should('be.hidden');
cy.get('.cvat-opencv-control-popover').should('have.attr', 'style').and('include', 'pointer-events: none');
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure we should test that the popovers are closed now.
At least check for pointer-events seems to be extra.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this check, there is a high probability of getting error AssertionError: Timed out retrying after 25000ms: Expected to find element: `.*-popover-visible`, but never found it.

Removed heck for pointer-events.

@dvkruchinin dvkruchinin force-pushed the dkru/fix-tests-migrating-antd branch from 17eef1b to 821bce4 Compare September 3, 2021 11:48
@nmanovic nmanovic requested a review from ActiveChooN September 6, 2021 13:13
@nmanovic
Copy link
Contributor

nmanovic commented Sep 7, 2021

@dvkruchinin , could you please fix the conflict?

@ActiveChooN , could you please review ASAP?

ActiveChooN
ActiveChooN previously approved these changes Sep 7, 2021
Copy link
Contributor

@ActiveChooN ActiveChooN left a comment

Choose a reason for hiding this comment

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

LGTM

@dvkruchinin
Copy link
Contributor Author

Сonflicts have been fixed.

@nmanovic
Copy link
Contributor

nmanovic commented Sep 7, 2021

@dvkruchinin , could you please look at the failed test?

@dvkruchinin
Copy link
Contributor Author

could you please look at the failed test?

Yes. I'm doing it now.

@dvkruchinin dvkruchinin force-pushed the dkru/fix-tests-migrating-antd branch from b351454 to ea7b45f Compare September 7, 2021 13:26
Copy link
Contributor

@nmanovic nmanovic left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -31,6 +31,7 @@ module.exports = (env) => ({
host: process.env.CVAT_UI_HOST || 'localhost',
port: 3000,
historyApiFallback: true,
host: process.env.CVAT_UI_HOST || 'localhost',
Copy link
Contributor

Choose a reason for hiding this comment

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

@dvkruchinin , do we need the line two times?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, I am not the author of this change.
@ActiveChooN
Can you please tell if it should be like this? If not, I will delete the extra line.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I will merge to unblock the PR. Thanks.

@nmanovic nmanovic merged commit 1ecb44a into cvat-ai:develop Sep 13, 2021
@Marishka17 Marishka17 mentioned this pull request Sep 15, 2021
8 tasks
nmanovic pushed a commit that referenced this pull request Sep 16, 2021
bsekachev added a commit that referenced this pull request Sep 20, 2021
bsekachev pushed a commit that referenced this pull request Sep 24, 2021
…3694)

* Revert "Revert "Updating tests for migration to a new version of antd (#3623)""

This reverts commit 0fd8b5d.

* Fixed issue with annotations uploading

* Fixed menu on annotation page

* Tried to fix: Cannot read property nodeType of null

* Updated deps

* Fixed a couple of issues in menu

* Fixed import after updating antd

* Update cvat-ui/webpack.config.js

Co-authored-by: Dmitry Kalinin <[email protected]>

Co-authored-by: Dmitry Kalinin <[email protected]>
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