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.

Sign up for GitHub

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

Jump to bottom

TSDB: More tests #78208

Merged
merged 3 commits into from
Oct 4, 2021
Merged

TSDB: More tests #78208

merged 3 commits into from
Oct 4, 2021

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Sep 22, 2021

This adds more tests copied from the our original TSDB prototype in
PR #75638 that are still applicable time series mode indices. There are
a bunch of disabled assertions because we are not yet generating _tsid
but the non-disabled assertions are useful. And we will soon be
generating the _tsid so we can re-enable those assertions.

@nik9000 nik9000 requested review from imotov and csoulios September 22, 2021 17:57
@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Sep 22, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)

@nik9000
TSDB: More tests
1a00ee4 
This adds more tests copied from the our original TSDB prototype in
PR elastic#75638 that are still applicable time series mode indices. There are
a bunch of disabled assertions because we are not yet generating `_tsid`
but the non-disabled assertions are useful. And we will soon be
generating the `_tsid` so we can re-enable those assertions.
imotov
imotov approved these changes Sep 22, 2021
View reviewed changes
Copy link
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

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

LGTM, I would only suggest removing the commented out tests or uncommenting them and placing them under skip clause. Committing such an amount of commented out code is a bit weird to me.

@nik9000
Copy link
Member Author

nik9000 commented Sep 22, 2021

LGTM, I would only suggest removing the commented out tests or uncommenting them and placing them under skip clause. Committing such an amount of commented out code is a bit weird to me.

Yeah. I'm not a fan of it. I kept them in because wanted to preserve the work I'd done in the prototype so we could grab it back. But I think we don't need commented out copies.

csoulios
csoulios approved these changes Sep 23, 2021
View reviewed changes
Copy link
Contributor

@csoulios csoulios left a comment

Choose a reason for hiding this comment

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

So many tests. I love them!

LGTM.

@imotov imotov mentioned this pull request Sep 23, 2021
Closed
@nik9000 nik9000 added >test Issues or PRs that are addressing/adding tests and removed v7.16.0 labels Oct 1, 2021
@nik9000 nik9000 merged commit bff56e1 into elastic:master Oct 4, 2021
@wchaparro wchaparro assigned nik9000 and unassigned nik9000 Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csoulios csoulios csoulios approved these changes

@imotov imotov imotov approved these changes

Assignees
No one assigned
Labels
:StorageEngine/TSDB You know, for Metrics Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test Issues or PRs that are addressing/adding tests v8.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nik9000 @elasticmachine @imotov @csoulios @jakelandis