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

Add m3ninx query prop tests #885

Merged
merged 1 commit into from
Sep 18, 2018
Merged

Conversation

prateek
Copy link
Collaborator

@prateek prateek commented Sep 6, 2018

Pending:

  • address regexp anchoring issue for simple v fst segment
  • gopter SliceOf reflection issue during query gen

@codecov
Copy link

codecov bot commented Sep 6, 2018

Codecov Report

Merging #885 into master will increase coverage by 22.92%.
The diff coverage is 57.14%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #885       +/-   ##
===========================================
+ Coverage    55.8%   78.73%   +22.92%     
===========================================
  Files         392      397        +5     
  Lines       33328    33523      +195     
===========================================
+ Hits        18599    26394     +7795     
+ Misses      13067     5317     -7750     
- Partials     1662     1812      +150
Flag Coverage Δ
#dbnode 81.3% <ø> (+11.66%) ⬆️
#m3ninx 74.84% <57.14%> (+53.21%) ⬆️
#query 69.19% <ø> (+60.84%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cf04be...c7b13ab. Read the comment docs.

@prateek prateek force-pushed the prateek/m3ninx/proptest-queries branch from 4c2fc94 to e6506fd Compare September 6, 2018 19:57
@prateek prateek changed the title [WIP] Add m3ninx query prop tests Add m3ninx query prop tests Sep 6, 2018
@prateek prateek force-pushed the prateek/m3ninx/proptest-queries branch 2 times, most recently from a8d2f77 to 91d80d0 Compare September 6, 2018 21:18
)

// CompileRegex compiles the provided regexp.
func CompileRegex(r []byte) (CompiledRegex, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey as we briefly discussed, could we instead use validation and just ensure that callers have fully anchored their regexps?

It is more explicit that way, rather than coercing their regexps into being fully anchored.

I don't mind too much either way, but that seems less magical just responding with an error saying "hey please fully anchor your regexp with "^pattern$".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done in #902

return nil, errReaderNilRegex
}

pl, err := r.segment.matchRegexp(field, compileRE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Instead perhaps just return r.segment.matchRegexp(field, compileRE)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will do

@@ -0,0 +1,62 @@
// Copyright (c) 2017 Uber Technologies, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

2018

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will do

@prateek prateek force-pushed the prateek/m3ninx/proptest-queries branch from 91d80d0 to 26f4ff7 Compare September 13, 2018 16:43
@prateek prateek changed the base branch from master to prateek/m3ninx/rework-regexp-queries September 13, 2018 16:44
Copy link
Contributor

@richardartoul richardartoul left a comment

Choose a reason for hiding this comment

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

LGTM

simpleSeg := newTestMemSegment(t, lotsTestDocuments)
fstSeg := fst.ToTestSegment(t, simpleSeg, fstOptions)

properties.Property("Simple & FST Segments Query the same results", prop.ForAll(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah nice, this is pretty great.

Copy link
Collaborator

@robskillington robskillington left a comment

Choose a reason for hiding this comment

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

LGTM

@prateek prateek force-pushed the prateek/m3ninx/rework-regexp-queries branch from 4feeba8 to 004910f Compare September 18, 2018 02:53
@prateek prateek force-pushed the prateek/m3ninx/proptest-queries branch from 2528c57 to 47d648f Compare September 18, 2018 03:27
@prateek prateek changed the base branch from prateek/m3ninx/rework-regexp-queries to master September 18, 2018 03:27
@prateek prateek merged commit 0cf190d into master Sep 18, 2018
@prateek prateek deleted the prateek/m3ninx/proptest-queries branch September 18, 2018 03:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants