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

Fixed default timestamp sort and added tests #35640

Merged
merged 8 commits into from May 3, 2019

Conversation

@igoristic
Copy link
Contributor

commented Apr 26, 2019

Summary

Resolves #34316

Removed reverse sorting caused by reduceRight

Checklist

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

igoristic added 2 commits Apr 26, 2019
@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

@igoristic

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

retest

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

@igoristic

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

retest

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

@igoristic igoristic requested a review from justinkambic Apr 30, 2019

@igoristic igoristic marked this pull request as ready for review Apr 30, 2019

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

@justinkambic
Copy link
Contributor

left a comment

This mostly LGTM - I did leave one question.

Was there any functional review you wanted me to perform? At this point I've only done code review.

const hit = response.hits.hits[0];
const version = ['6.6.2', '7.0.0-rc1', '6.7.1'];

for (let i = 0, l = version.length; i < l; ++i) {

This comment has been minimized.

Copy link
@justinkambic

justinkambic May 1, 2019

Contributor

This is a nit-picky suggestion but given that we are only referencing l as part of the loop's evaluation, can we drop the declaration of l altogether and simply have for (let i = 0; i < version.length; ++i)? In the code we currently have, l is essentially a constant that is only referenced once, so I don't think we lose any readability by referencing the array's length directly.

Again, just a suggestion, I leave it up to your discretion.

This comment has been minimized.

Copy link
@igoristic

igoristic May 1, 2019

Author Contributor

I guess since this is a small loop it wouldn't make any difference. But, there is actually a performance gain doing it this way since Array.length is only calculated once (and not per every iteration).

I'll keep it this way for now since this is just a unit test, but I'll be sure to favor readability in the future

This comment has been minimized.

Copy link
@justinkambic

justinkambic May 1, 2019

Contributor

That's fine! 👍

igoristic added 2 commits May 2, 2019

@igoristic igoristic requested a review from justinkambic May 2, 2019

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

@justinkambic
Copy link
Contributor

left a comment

LGTM

@igoristic igoristic added v7.1.0 and removed v7.2.0 labels May 3, 2019

@igoristic igoristic merged commit 3d3e071 into elastic:master May 3, 2019

43 checks passed

API integration tests node scripts/functional_tests --config test/api_integration/config.js --bail --debug
Details
Browser tests yarn run grunt test:browser-ci
Details
Build kbn_tp_sample_panel_action yarn build
Details
CLA All commits in pull request signed
Details
Check core API changes node scripts/check_core_api_changes
Details
Check file casing node scripts/check_file_casing --quiet
Details
Functional tests / Group 1 yarn run grunt run:functionalTests_ciGroup1
Details
Functional tests / Group 10 yarn run grunt run:functionalTests_ciGroup10
Details
Functional tests / Group 11 yarn run grunt run:functionalTests_ciGroup11
Details
Functional tests / Group 12 yarn run grunt run:functionalTests_ciGroup12
Details
Functional tests / Group 2 yarn run grunt run:functionalTests_ciGroup2
Details
Functional tests / Group 3 yarn run grunt run:functionalTests_ciGroup3
Details
Functional tests / Group 4 yarn run grunt run:functionalTests_ciGroup4
Details
Functional tests / Group 5 yarn run grunt run:functionalTests_ciGroup5
Details
Functional tests / Group 6 yarn run grunt run:functionalTests_ciGroup6
Details
Functional tests / Group 7 yarn run grunt run:functionalTests_ciGroup7
Details
Functional tests / Group 8 yarn run grunt run:functionalTests_ciGroup8
Details
Functional tests / Group 9 yarn run grunt run:functionalTests_ciGroup9
Details
Internationalization check node scripts/i18n_check --ignore-missing
Details
Interpreter functional tests node scripts/functional_tests --config test/interpreter_functional/config.js --bail --debug --kibana-install-dir ./build/oss/kibana-8.0.0-SNAPSHOT-linux-x86_64
Details
Jest integration tests yarn run grunt test:jest_integration
Details
Jest tests yarn run grunt test:jest
Details
Licenses yarn run grunt licenses
Details
Mocha tests node scripts/mocha
Details
Plugin functional tests node scripts/functional_tests --config test/plugin_functional/config.js --bail --debug --kibana-install-dir ./build/oss/kibana-8.0.0-SNAPSHOT-linux-x86_64
Details
Project tests yarn run grunt test:projects
Details
Server tests yarn run grunt test:server
Details
Type check node scripts/type_check
Details
TypeScript - all files belong to a TypeScript project node scripts/check_ts_projects
Details
Verify NOTICE.txt node scripts/notice --validate
Details
Verify dependency versions yarn run grunt verifyDependencyVersions
Details
X-Pack Functional tests / Group 1 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup1/node/immutable/install/kibana --include-tag ciGroup1
Details
X-Pack Functional tests / Group 2 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup2/node/immutable/install/kibana --include-tag ciGroup2
Details
X-Pack Functional tests / Group 3 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup3/node/immutable/install/kibana --include-tag ciGroup3
Details
X-Pack Functional tests / Group 4 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup4/node/immutable/install/kibana --include-tag ciGroup4
Details
X-Pack Functional tests / Group 5 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup5/node/immutable/install/kibana --include-tag ciGroup5
Details
X-Pack Functional tests / Group 6 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup6/node/immutable/install/kibana --include-tag ciGroup6
Details
X-Pack Functional tests / Group 7 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup7/node/immutable/install/kibana --include-tag ciGroup7
Details
X-Pack Jest node scripts/jest --ci --no-cache --verbose
Details
X-Pack Mocha yarn test
Details
eslint node scripts/eslint --no-cache
Details
kibana-ci Build finished.
Details
sasslint node scripts/sasslint
Details

@igoristic igoristic deleted the igoristic:34316 branch May 3, 2019

igoristic added a commit to igoristic/kibana that referenced this pull request May 3, 2019
Fixed default timestamp sort and added tests (elastic#35640)
* Fixed default timestamp sort and added tests

* Removed duplicate functionality

* Fixed unit tests

* Removed debug code

* Update helpers.js
igoristic added a commit to igoristic/kibana that referenced this pull request May 3, 2019
Fixed default timestamp sort and added tests (elastic#35640)
* Fixed default timestamp sort and added tests

* Removed duplicate functionality

* Fixed unit tests

* Removed debug code

* Update helpers.js
igoristic added a commit that referenced this pull request May 3, 2019
Fixed default timestamp sort and added tests (#35640) (#36042)
* Fixed default timestamp sort and added tests

* Removed duplicate functionality

* Fixed unit tests

* Removed debug code

* Update helpers.js
igoristic added a commit that referenced this pull request May 3, 2019
Fixed default timestamp sort and added tests (#35640) (#36041)
* Fixed default timestamp sort and added tests

* Removed duplicate functionality

* Fixed unit tests

* Removed debug code

* Update helpers.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.