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

[IN:9509] reduces the page size for Test Cases #59

Merged
merged 3 commits into from
Jun 10, 2024
Merged

Conversation

brgruber92
Copy link
Collaborator

Fixes [IN:9509] slow Test Case retrieval by reducing the page size for this artifact to 200 records. In my tests, it took around 1 minute to retrieve the page.

Copy link
Member

@simonhbor simonhbor left a comment

Choose a reason for hiding this comment

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

Everything looks good apart from one thing I am confused about

@@ -2237,7 +2249,7 @@ async function sendToSpira(model, fieldTypeEnums, isUpdate) {
} else {
return await Excel.run({ delayForCellEdit: true }, function (context) {
var sheet = context.workbook.worksheets.getActiveWorksheet(),
sheetRange = sheet.getRangeByIndexes(1, 0, MAX_ROWS_PER_PAGE, fields.length);
sheetRange = sheet.getRangeByIndexes(1, 0, MAX_ROWS_PER_PAGE_MAX, fields.length);
Copy link
Member

Choose a reason for hiding this comment

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

Why is this using the MAX_ROWS_PER_PAGE_MAX var and not the dynamic MAX_ROWS_PER_PAGE?
I think this is for loading the template, but not sure why it will always be 2000 rows?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because here we are not loading any data from the API, but rather sending data to Spira. Excel requires us to provide a range number for the page. Ideally, this should be "send all the data in this spreadsheet" - since we don't have that infinite row option, we stick with the max 2,000 rows limit. Does it make sense?

Copy link
Member

Choose a reason for hiding this comment

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

So here we are finding the rows to send to Spira from Excel?

  1. What do we do for eg Incidents if people have added more than 2000 rows of data?
  2. For Test cases with test steps, they may have entered a lot less than 200 test cases in 2000 rows. So how should we handle that?

Basically, this approach may cut off what data is uploaded to Excel and users will not know. The easiest answer is to select a much bigger number I think. Maybe 64000? Unlike Sheets, Excel always has a lot of rows so we should be safe in determining that but should check on web and desktop

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That makes sense. I modified it to look for 64k rows when sending. I believe this does not impact performance, as it stops scanning when it detects an empty row.

@simonhbor simonhbor assigned brgruber92 and unassigned simonhbor Jun 4, 2024
@brgruber92 brgruber92 assigned simonhbor and unassigned brgruber92 Jun 5, 2024
Copy link
Member

@simonhbor simonhbor left a comment

Choose a reason for hiding this comment

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

I think we need changes still when sending data to Spira for Excel

@@ -2237,7 +2249,7 @@ async function sendToSpira(model, fieldTypeEnums, isUpdate) {
} else {
return await Excel.run({ delayForCellEdit: true }, function (context) {
var sheet = context.workbook.worksheets.getActiveWorksheet(),
sheetRange = sheet.getRangeByIndexes(1, 0, MAX_ROWS_PER_PAGE, fields.length);
sheetRange = sheet.getRangeByIndexes(1, 0, MAX_ROWS_PER_PAGE_MAX, fields.length);
Copy link
Member

Choose a reason for hiding this comment

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

So here we are finding the rows to send to Spira from Excel?

  1. What do we do for eg Incidents if people have added more than 2000 rows of data?
  2. For Test cases with test steps, they may have entered a lot less than 200 test cases in 2000 rows. So how should we handle that?

Basically, this approach may cut off what data is uploaded to Excel and users will not know. The easiest answer is to select a much bigger number I think. Maybe 64000? Unlike Sheets, Excel always has a lot of rows so we should be safe in determining that but should check on web and desktop

@simonhbor simonhbor assigned brgruber92 and unassigned simonhbor Jun 7, 2024
@brgruber92 brgruber92 assigned simonhbor and unassigned brgruber92 Jun 7, 2024
Copy link
Member

@simonhbor simonhbor left a comment

Choose a reason for hiding this comment

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

changes make sense

@simonhbor simonhbor merged commit 7e53dee into master Jun 10, 2024
2 checks passed
@simonhbor simonhbor deleted the IN9509-TC_pageSize branch June 10, 2024 12:40
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.

None yet

2 participants