-
Notifications
You must be signed in to change notification settings - Fork 15
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
Improve AT version and browser version queries #623
Conversation
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.
This is a huge amount of work, the pages are zooming! Thanks for this investigation and major optimization @Paul-Clue @alflennik 👍🏽
@@ -194,9 +168,9 @@ const getTestPlanReports = async ( | |||
testPlanRunAttributes = TEST_PLAN_RUN_ATTRIBUTES, | |||
testPlanVersionAttributes = TEST_PLAN_VERSION_ATTRIBUTES, | |||
atAttributes = AT_ATTRIBUTES, | |||
atVersionAttributes = AT_VERSION_ATTRIBUTES, | |||
atVersionAttributes = null, |
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.
Since this is no longer referenced in atAssociation()
, this can be removed. The atAssociation
function call in this function can also be updated and in other functions throughout this file.
browserAttributes = BROWSER_ATTRIBUTES, | ||
browserVersionAttributes = BROWSER_VERSION_ATTRIBUTES, | ||
browserVersionAttributes = null, |
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.
Same comment as above. The browserAssociation
function call in this function should also be updated and in other functions throughout this file.
server/models/index.js
Outdated
@@ -17,7 +17,7 @@ sequelize = new Sequelize( | |||
ssl: true, | |||
native: true | |||
}, | |||
logging: false // console.log // eslint-disable-line | |||
logging: false // console.log // eslint-disable-line |
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.
nit: this line needs to be reformatted
const atLoader = AtLoader(); | ||
const browserLoader = BrowserLoader(); | ||
|
||
return { user, atLoader, browserLoader }; |
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.
Without the context of this PR's description, it does feel somewhat out of place, for atLoader
and browserLoader
to be included here. Perhaps a comment can be added here?
@@ -75,45 +75,19 @@ const testPlanVersionAssociation = testPlanVersionAttributes => ({ | |||
* @param {string[]} atVersionAttributes - AT version attributes | |||
* @returns {{association: string, attributes: string[]}} | |||
*/ | |||
const atAssociation = (atAttributes, atVersionAttributes) => ({ | |||
const atAssociation = atAttributes => ({ |
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.
Functions calling atAssociation
in this file need to be updated
}); | ||
|
||
/** | ||
* @param {string[]} browserAttributes - Browser attributes | ||
* @param {string[]} browserVersionAttributes - Browser version attributes | ||
* @returns {{association: string, attributes: string[]}} | ||
*/ | ||
const browserAssociation = (browserAttributes, browserVersionAttributes) => ({ | ||
const browserAssociation = browserAttributes => ({ |
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.
Functions calling browserAssociation
in this file need to be updated
@@ -64,45 +64,19 @@ const testPlanRunAssociation = (testPlanRunAttributes, userAttributes) => ({ | |||
* @param {string[]} atVersionAttributes - AT version attributes | |||
* @returns {{association: string, attributes: string[]}} | |||
*/ | |||
const atAssociation = (atAttributes, atVersionAttributes) => ({ | |||
const atAssociation = atAttributes => ({ |
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.
Same comment as before
}); | ||
|
||
/** | ||
* @param {string[]} browserAttributes - Browser attributes | ||
* @param {string[]} browserVersionAttributes - Browser version attributes | ||
* @returns {{association: string, attributes: string[]}} | ||
*/ | ||
const browserAssociation = (browserAttributes, browserVersionAttributes) => ({ | ||
const browserAssociation = browserAttributes => ({ |
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.
Same comment as before
@Paul-Clue @alflennik amazing work! Pending @howard-e's change requests, this is good to go! |
…r TestPlanReportService
…r TestPlanRunService
…r TestPlanVersionService
#624) * Address a case where ManageTestQueue can load an empty list of atVersions on first load * Remove unnecessary closing parenthesis on TestQueueRow when displaying the amount of tests completed * Fix spacing between elements for the 'Review Conflicts Status Bar' on TestRun page (bootstrap classnames have changed) * Highlight 'Test Queue' in the nav bar when Test Run page is open * Fix document ordering of the AT details, the browser details and the edit button on the Test Run page (change from 'AT > Edit Button > Browser' to 'AT > Browser > Edit Button') * Restructure the TestRun component's loading strategy to make it more "React-like" and hopefully more readable. Some underlying issues were exposed here with the increase in load speed (with the previous strategy, there would be a chance for a Test to not be rendered the first time it was loaded) * Address 'undefined' being displayed from AT and Browser versions when creating an issue from Report Page; also account for a structure to include link to the report page * Ignore cache when viewing TestRun page
This PR improves the test queue query speed by 69X and improves the querying speeds for all the rest of pages as well.
includes
query from theatAssociation
andbrowserAssociation
functions in theTestPlanVersionService
andTestPlanReportService
files. Theincludes
query is what was causing the cartesian product problem.AtLoader
andBrowserLoader
that fetches all of the AT versions and browser versions, respectively.