-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 task - regression testing to export tasks #5519
Conversation
@bsekachev , could you please look at the PR? |
}); | ||
|
||
context('Export task inner.', { browser: '!firefox' }, () => { | ||
const caseId = '999'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should refer to real issue. If no issue exist, this does not make sense.
cy.deleteTask(taskName); | ||
}); | ||
|
||
describe(`Testing "${taskName}"`, () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to be more specific here. It is not clear what we are testing. Using string template is not the best option here because it makes code more difficult to read and understand.
it('Open a task', () => { | ||
cy.openTask(taskName); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be a part of before
probably, because we don't test opening a task here
// Start writing your Cypress tests below! | ||
// If you're unfamiliar with how Cypress works, | ||
// check out the link below and learn how to write your first test: | ||
// https://on.cypress.io/writing-first-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to put a lincese header here instead of default Cypress text:
// Copyright (C) 2022 CVAT.ai Corporation
//
// SPDX-License-Identifier: MIT
|
||
/// <reference types="cypress" /> | ||
|
||
Cypress.Commands.add('exportInnerTask', ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a dedicated files for Cypress commands definition:
tests/cypress/support/*.js
order, | ||
imageClick = false, | ||
}) => { | ||
const orderId = Number.isNaN(parseInt(order, 10)) ? 0 : parseInt(order, 10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const orderId = Number.isNaN(parseInt(order, 10)) ? 0 : parseInt(order, 10); | |
const orderId = +order || 0; |
But generally, if we need an integer here, passing a string to this command does not make sense
const imagesFolder = `cypress/fixtures/${imageFileName}`; | ||
const directoryToArchive = imagesFolder; | ||
const newLabelName = 'person'; | ||
const exportOrders = [0, 1, 2]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choosing an export format by its order is not robust option I think. Order can be changed easily.
Better to choose by name
@@ -0,0 +1,138 @@ | |||
// Start writing your Cypress tests below! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Please check comments from the previous file, many of them applicable here
- A lot of code dublication, I would prefer rework it somehow to reduce it. Maybe to combine these two tests into the one.
Looks like the test has not been run on CI because of configuration of |
@AparinAA , thanks for PR. I will close it. |
OK. I can fix the wrong if needed |
Added test to #5396