Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/20538 )
Change subject: IMPALA-12415: Implement tests for graphical query timeline in webUI ...................................................................... Patch Set 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/20538/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20538/1//COMMIT_MSG@14 PS1, Line 14: nit: could you describe the directory structure of testing scripts? http://gerrit.cloudera.org:8080/#/c/20538/1//COMMIT_MSG@15 PS1, Line 15: NodeJS binaries are stored within the IMPALA_TOOLCHAIN directory. All of packages under toolchain directory are built into Impala toolchain. It's better to add NodeJS to Impala toolchain. http://gerrit.cloudera.org:8080/#/c/20538/1/tests/run-js-tests.sh File tests/run-js-tests.sh: http://gerrit.cloudera.org:8080/#/c/20538/1/tests/run-js-tests.sh@26 PS1, Line 26: NODEJS_LIB_PATH="${IMPALA_TOOLCHAIN}/node-${NODEJS_VERSION}" It's better to add the nodejs package to Impala toolchain so that developers don't need to download it directly from nodejs.org. To add it to toolchain, you need to work on a separate patch to update toolchain first, then bump up toolchain version. Impala toolchain is maintained in a separate repo: https://github.com/cloudera/native-toolchain http://gerrit.cloudera.org:8080/#/c/20538/1/tests/run-js-tests.sh@47 PS1, Line 47: cd ${IMPALA_HOME}/www/scripts/ save the current directory and restore it after running js tests. http://gerrit.cloudera.org:8080/#/c/20538/1/tests/run-js-tests.sh@50 PS1, Line 50: $NPM install what's the default target directory to install? http://gerrit.cloudera.org:8080/#/c/20538/1/tests/run-js-tests.sh@53 PS1, Line 53: $NPM test Is it possible to specify the directory of testing suite? If yes, then we don't need to change current directory. http://gerrit.cloudera.org:8080/#/c/20538/1/www/scripts/package.json File www/scripts/package.json: http://gerrit.cloudera.org:8080/#/c/20538/1/www/scripts/package.json@14 PS1, Line 14: nit: keep indent consistent http://gerrit.cloudera.org:8080/#/c/20538/1/www/scripts/query_timeline/chart_commons.test.js File www/scripts/query_timeline/chart_commons.test.js: http://gerrit.cloudera.org:8080/#/c/20538/1/www/scripts/query_timeline/chart_commons.test.js@21 PS1, Line 21: nit: add comments to describe what are to be tested. http://gerrit.cloudera.org:8080/#/c/20538/1/www/scripts/query_timeline/fragment_diagram.test.js File www/scripts/query_timeline/fragment_diagram.test.js: http://gerrit.cloudera.org:8080/#/c/20538/1/www/scripts/query_timeline/fragment_diagram.test.js@20 PS1, Line 20: nit: add comments to describe what to be tested. http://gerrit.cloudera.org:8080/#/c/20538/1/www/scripts/query_timeline/fragment_diagram.test.js@30 PS1, Line 30: ` should we use ' here and other places instead of `? -- To view, visit http://gerrit.cloudera.org:8080/20538 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0caf0a0beee23821f78c0b3fe1aeb7dbf92d6a3e Gerrit-Change-Number: 20538 Gerrit-PatchSet: 1 Gerrit-Owner: Surya Hebbar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Tue, 10 Oct 2023 01:04:53 +0000 Gerrit-HasComments: Yes
