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

Cypress test. Connected file share. #3764

Conversation

dvkruchinin
Copy link
Contributor

Motivation and context

Added new test to check the functionality of creating a task using "Connected file share".
Check PR #3733 (https://github.com/dvkruchinin/cvat/runs/3792189841?check_suite_focus=true#step:5:4627)

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

@Marishka17 Marishka17 self-requested a review October 5, 2021 08:44
@Marishka17 Marishka17 self-assigned this Oct 5, 2021
Comment on lines 75 to 79
cy.exec(`docker exec -i cvat /bin/bash -c "find ~/share -name "*.png" -type f"`)
.then((findFilesCommand) => {
// [image_case_107_2.png, image_case_107_3.png]
expect(findFilesCommand.stdout.split('\n').length).to.be.eq(2);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Why need this check if the previous command

cy.exec(`mv ${assetLocalPath}/${stdoutToList[0]} ${assetLocalPath}/${stdoutToList[0]}.bk`)
                .then((fileRenameCommand) => {
                    expect(fileRenameCommand.code).to.be.eq(0);
                });

will return code 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make sure that the file has been renamed. And there are only 2 images used in the task. Checking the exit code of the rename command has been added just in case.

});
});

it('Check "Fix problem with getting cloud storages in Firefox".', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

So, if this test doesn't have common with cloud storage features, maybe better to use a such message?

Suggested change
it('Check "Fix problem with getting cloud storages in Firefox".', () => {
it('Check "Fix DataCloneError: The object could not be cloned in Firefox".', () => {

or

Suggested change
it('Check "Fix problem with getting cloud storages in Firefox".', () => {
it('Check "Fix problem with getting share data for the task when data not available more in Firefox".', () => {

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 chose the second option. Applied locally.
Right now GitHub is having problems with actions. The jobs will start after the problems are fixed. https://www.githubstatus.com/

Marishka17
Marishka17 previously approved these changes Oct 6, 2021
.should('be.visible')
.within(() => {
cy.get('[aria-label="plus-square"]').click();
cy.get('[title]').should('have.length', 4); // Also "root"
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this check? It can have any other name actually and only specified in UI as "root". At the same time the name does not affect UX anyhow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Slightly overworked. I think this check is still needed. To check for the absence of extra lines.

@bsekachev bsekachev merged commit e8b3284 into cvat-ai:develop Oct 8, 2021
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.

3 participants