Updated Perf Test display with parent and children builds separately #52

sophiaxu0424 · 2019-03-12T21:36:47Z

- Perf Test should display the parent builds in the first layer, by clicking each parent build's link would give information of all its child builds.

Issue: #24

piyush286

Thanks Sophia for working on this issue. Please update the typo "seperately" in the title. I'll review the changes later today.

llxia · 2019-03-13T13:23:43Z

test-result-summary-client/src/Build/TopLevelBuilds.jsx

-                        </div>;
-                    }
+                    return <div>
+                        <Link to={{ pathname: '/buildDetail', search: params( { parentId: value._id } ) }}


We need to make sure the updated code will work for other types of tests.

Modified as required. Please check.

@sophiaxu0424 Please make sure to validate your changes with non-perf tests as well. You can choose 1-2 pipelines from the build monitoring list of the currently deployed TRSS and check whether those jobs can be parsed in the DB and viewed on TRSC as expected.

I think this will break for builds that contain tests directly (i.e., Daily-ODM). We could keep the original code, and add else block for line 76. In the else block, we can link to /buildDetail.

Yes, that's right, changed as required.

llxia · 2019-03-13T13:33:31Z

TestResultSummaryService/DataManager.js

+            if ( response && response.insertedCount === 1 ) {
+                buildOutputId = response.insertedIds[0];
+            }
+            update.buildOutputId = buildOutputId;


If there is tests obj return, the test output will be stored in line 106. There is no need to store the output again. If the plan is to store the perf test output, then the perf parser should be updated to store the value as testOutput.

Yes, I agree, changed it back already.

llxia · 2019-03-13T13:38:17Z

test-result-summary-client/src/Build/BuildTable.jsx

+                        style={{ color: resultColor }}> {value.buildName} # {value.buildNum} </Link>;
+                } else {
+                    return <Link to={{ pathname: '/output/build', search: params( { id: value._id } ) }}
+                        style={{ color: resultColor }}> {value.buildName} # {value.buildNum} </Link>;


There is no special treatment for Perf. Could we just remove the if else altogether?

Code changed as required. Please check.

sophiaxu0424 · 2019-03-15T15:43:08Z

@llxia @piyush286 updated as required. But having a new issue with conflict from above, not quite sure what to do. Would you please provide some information. Thank you.

piyush286 · 2019-03-15T17:15:49Z

@sophiaxu0424 Let's discuss that issue on Webex!

piyush286

@llxia Can you please review the changes? Thanks!

piyush286 · 2019-03-15T20:17:54Z

test-result-summary-client/src/Build/BuildTable.jsx

+                    if ( value.tests[0]._id ) {
+                        return <div>
+                            <Link to={{ pathname: '/output/test', search: params( { id: value.tests[0]._id } ) }}
+                                style={{ color: result === "PASSED" ? "#2cbe4e" : ( result === "FAILED" ? "#f50" : "#DAA520" ) }}>


Don't we just have 2 conditions: PASSED OR FAILED for performance tests?
https://github.com/AdoptOpenJDK/openjdk-test-tools/blob/8006133d8f660ce49c1c0345ab7ac241d49eec8e/TestResultSummaryService/parsers/BenchmarkParser.js#L148

Not sure if we need DAA520. I notice that it was there earlier in TopLevelBuilds.jsx as well.

#DAA520 and #f50 are the background colors respect to the status of the result. Keep it won't be a big problem.

We have other conditions for the test result (i.e., SKIPPED), so #DAA520 is needed.

piyush286 · 2019-03-15T20:28:10Z

test-result-summary-client/src/Build/TopLevelBuilds.jsx

-                        </div>;
-                    }
+                    return <div>
+                        <Link to={{ pathname: '/buildDetail', search: params( { parentId: value._id } ) }}


@sophiaxu0424 Please make sure to validate your changes with non-perf tests as well. You can choose 1-2 pipelines from the build monitoring list of the currently deployed TRSS and check whether those jobs can be parsed in the DB and viewed on TRSC as expected.

piyush286 · 2019-03-20T01:12:58Z

test-result-summary-client/src/Build/BuildTable.jsx

+                    let result = value.tests[0].testResult;
+                    if ( value.tests[0]._id ) {
+                        return <div>
+                            <Link to={{ pathname: '/output/test', search: params( { id: value.tests[0]._id } ) }}


Related issue for this: #70

This looks like is a bug with the old design since we would only show the the output of the first iteration (value.tests[0]._id) when the user clicks on a Jenkins build even though that Jenkins build may have multiple iterations, which won't be displayed. This problem will be fixed with the new design.

We can address this issue in #70.

I agree, I have tested it using FVT (non-perf) build type before making this pull request. But since I am having a problem logging to the internal Jenkins server, I would test it again after fixing the logging problem.:)

Instead of adding special code for perf, I am wondering if we can leverage allTestsInfo directly.

As I noticed, the value type could be "Build" "Perf" and "Test', so if the first condition(either "Build" or "Perf") is not satisfied, it will go to the allTestInfo (line 52).

llxia · 2019-03-20T13:46:56Z

Sure, I will review this shortly. I was confused by discussing on Webex comment.

llxia · 2019-03-20T15:07:43Z

test-result-summary-client/src/Build/BuildTable.jsx

+                    if ( value.tests[0]._id ) {
+                        return <div>
+                            <Link to={{ pathname: '/output/test', search: params( { id: value.tests[0]._id } ) }}
+                                style={{ color: result === "PASSED" ? "#2cbe4e" : ( result === "FAILED" ? "#f50" : "#DAA520" ) }}>


We have other conditions for the test result (i.e., SKIPPED), so #DAA520 is needed.

llxia · 2019-03-20T15:43:44Z

test-result-summary-client/src/Build/BuildTable.jsx

+                    let result = value.tests[0].testResult;
+                    if ( value.tests[0]._id ) {
+                        return <div>
+                            <Link to={{ pathname: '/output/test', search: params( { id: value.tests[0]._id } ) }}


Instead of adding special code for perf, I am wondering if we can leverage allTestsInfo directly.

llxia · 2019-03-20T15:44:42Z

test-result-summary-client/src/Build/TopLevelBuilds.jsx

-                        </div>;
-                    }
+                    return <div>
+                        <Link to={{ pathname: '/buildDetail', search: params( { parentId: value._id } ) }}


I think this will break for builds that contain tests directly (i.e., Daily-ODM). We could keep the original code, and add else block for line 76. In the else block, we can link to /buildDetail.

llxia · 2019-03-20T15:59:34Z

test-result-summary-client/src/Build/BuildDetail.jsx

@@ -69,6 +69,7 @@ export default class BuildDetail extends Component {
                    buildUrl: builds[i].buildUrl,
                    type: builds[i].type,
                    hasChildren: builds[i].hasChildren,
+                    tests: builds[i].tests ? builds[i].tests : null,


Is tests used?

Yes, tests is defined here for been used in buildTable.js (else if statement in line 37).

llxia · 2019-03-27T20:50:12Z

test-result-summary-client/src/Build/AllTestsInfo.jsx

@@ -17,11 +17,10 @@ export default class Build extends Component {

    async updateData() {
        const { buildId } = getParams( this.props.location.search );
-        const fetchBuild = await fetch( `/api/getAllTestsWithHistory?buildId=${buildId}&limit=5 `, {
+        const fetchBuild = await fetch( `/api/getAllTestsWithHistory?buildId=${buildId}&limit=1 `, {


The value for limit has to be passed in. If Perf, set the limit to 1. Otherwise, the limit is 5.

llxia · 2019-03-27T20:51:59Z

test-result-summary-client/src/Build/TopLevelBuilds.jsx

@@ -83,6 +83,12 @@ export default class TopLevelBuilds extends Component {
                                </Link>
                            </div>;
                        }
+                    } else if (value.parentId !== null){


I think we can use else here

sophiaxu0424

@llxia @piyush286 Updated the code as required, would you please have a review again? Thank you.

sophiaxu0424 · 2019-03-22T19:51:09Z

test-result-summary-client/src/Build/TopLevelBuilds.jsx

-                        </div>;
-                    }
+                    return <div>
+                        <Link to={{ pathname: '/buildDetail', search: params( { parentId: value._id } ) }}


Yes, that's right, changed as required.

llxia · 2019-03-28T15:20:20Z

test-result-summary-client/src/Build/AllTestsInfo.jsx

+        let limitNum = 5;
+        if ( build[0].type === "Perf" ) {
+            limitNum = 1;
+        }


An additional query to figure out the type is not necessary. When calling allTestsInfo (link below), we know the type, we can set the limit based on type and pass the limit as params.

https://github.com/AdoptOpenJDK/openjdk-test-tools/blob/cd800edf8d4b392d033611c4194d2707bd12acfb/test-result-summary-client/src/Build/BuildTable.jsx#L52

In AllTestsInfo.jsx, we can get the limit from the line 19 as following:

const { buildId, limit } = getParams( this.props.location.search );

test-result-summary-client/src/Build/TopLevelBuilds.jsx

- Test perf should display the parent builds in the first layer, by clicking each parent build's link would give information of all its child builds. Issue: #24 #70 adoptium/aqa-tests#850 Signed-off-by: sophiaxu0424 <xmh1989@my.yorku.ca>

sophiaxu0424 · 2019-04-03T17:54:31Z

@llxia I have updated the required changes, would you please take another review? Thanks.

llxia

LGTM

karianna added this to In progress in aqa-test-tools via automation Mar 12, 2019

karianna added the enhancement New feature or request label Mar 12, 2019

sophiaxu0424 changed the title ~~Updated Test Perf display with parent and children builds seperately~~ Updated Perf Test display with parent and children builds seperately Mar 13, 2019

aqa-test-tools automation moved this from In progress to Needs review Mar 13, 2019

piyush286 suggested changes Mar 13, 2019

View reviewed changes

sophiaxu0424 changed the title ~~Updated Perf Test display with parent and children builds seperately~~ Updated Perf Test display with parent and children builds separately Mar 13, 2019

llxia reviewed Mar 13, 2019

View reviewed changes

sophiaxu0424 changed the title ~~Updated Perf Test display with parent and children builds separately~~ Updated Perf Test display with parent and children builds separately -in progress Mar 14, 2019

sophiaxu0424 changed the title ~~Updated Perf Test display with parent and children builds separately -in progress~~ Updated Perf Test display with parent and children builds separately Mar 15, 2019

piyush286 approved these changes Mar 20, 2019

View reviewed changes

llxia requested changes Mar 22, 2019

View reviewed changes

sophiaxu0424 mentioned this pull request Mar 25, 2019

#https://github.com/AdoptOpenJDK/openjdk-tests/issues/850 #81

Closed

llxia reviewed Mar 27, 2019

View reviewed changes

sophiaxu0424 commented Mar 28, 2019

View reviewed changes

llxia requested changes Mar 28, 2019

View reviewed changes

llxia reviewed Mar 29, 2019

View reviewed changes

test-result-summary-client/src/Build/TopLevelBuilds.jsx Outdated Show resolved Hide resolved

llxia reviewed Mar 29, 2019

View reviewed changes

test-result-summary-client/src/Build/TopLevelBuilds.jsx Outdated Show resolved Hide resolved

aqa-test-tools automation moved this from Needs review to Reviewer approved Apr 4, 2019

llxia approved these changes Apr 4, 2019

View reviewed changes

llxia merged commit d4102dd into adoptium:master Apr 4, 2019

aqa-test-tools automation moved this from Reviewer approved to Done Apr 4, 2019

Updated Perf Test display with parent and children builds separately #52

Updated Perf Test display with parent and children builds separately #52

Conversation

sophiaxu0424 commented Mar 12, 2019 • edited

piyush286 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophiaxu0424 commented Mar 15, 2019

piyush286 commented Mar 15, 2019 • edited

piyush286 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophiaxu0424 Mar 22, 2019 • edited

Choose a reason for hiding this comment

llxia commented Mar 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llxia Mar 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophiaxu0424 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophiaxu0424 commented Apr 3, 2019

llxia left a comment

Choose a reason for hiding this comment

sophiaxu0424 commented Mar 12, 2019 •

edited

piyush286 commented Mar 15, 2019 •

edited

sophiaxu0424 Mar 22, 2019 •

edited

llxia Mar 27, 2019 •

edited