opt: directive that asserts a rule was used#29135
opt: directive that asserts a rule was used#29135craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
a30ae54 to
73a0740
Compare
RaduBerinde
left a comment
There was a problem hiding this comment.
opttester already has a ruleFromString
Reviewable status:
complete! 0 of 0 LGTMs obtained
pkg/sql/opt/testutils/opt_tester.go, line 96 at r1 (raw file):
// DisableRules is a set of rules that are not allowed to run. DisableRules map[opt.RuleName]struct{}
Should also be a RuleSet
73a0740 to
c3f7888
Compare
justinj
left a comment
There was a problem hiding this comment.
Oh great - didn't notice that.
Reviewable status:
pkg/sql/opt/testutils/opt_tester.go, line 96 at r1 (raw file):
Previously, RaduBerinde wrote…
Should also be a
RuleSet
Done.
RaduBerinde
left a comment
There was a problem hiding this comment.
LGTM.
Note that all flags are documented in the comment for RunCommand, please update it.
Reviewable status:
c3f7888 to
b73bdb5
Compare
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 1 of 3 files at r1, 2 of 2 files at r2.
Reviewable status:
andy-kimball
left a comment
There was a problem hiding this comment.
Reviewable status:
b73bdb5 to
a655e1b
Compare
|
TFTRs! Was going to put the additional directive changes on this PR but after some thought I'm going to split it out and merge this as-is. bors r+ |
Build failed (retrying...) |
Build failed (retrying...) |
Build failed (retrying...) |
|
Bors will never pass this, Lint is failing. I recommend waiting for GitHub CI to pass before asking bors to merge. |
|
bors r- |
Canceled |
Brought up in cockroachdb#29050 as a possibility to avoid the case of certain rules no longer applying. I think this is probably valuable in that it asserts we're testing what we want to test, but I do think it's a little invasive as far as cluttering up the test directives goes. Open to suggestions for how to possibly improve. Release note: None
a655e1b to
5870878
Compare
|
CI green now bors r+ |
|
still "wip"? |
|
renamed the PR - the commit message was updated before |
|
This just happens to be one of my frivolous peeves. I think a wip marker
can only hurt; it's frequently forgotten and I need more info somewhere
else (a review comment probably) to know what's in progress anyway.
…On Tue, Aug 28, 2018 at 4:51 PM Justin Jaffray ***@***.***> wrote:
renamed the PR - the commit message was updated before
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#29135 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAXBcfrPjHeK1rzX4rKkNYyjXoIP0ZhXks5uVa1KgaJpZM4WOQvr>
.
|
Build failed (retrying...) |
29135: opt: directive that asserts a rule was used r=justinj a=justinj Brought up in #29050 as a possibility to avoid the case of certain rules no longer applying. Kind of a strawman right now because I'm not sure * if this is the syntax we want, * how widely we want to use it, I've just added it to one file for the purposes of this PR. I think this is probably valuable in that it asserts we're testing what we want to test, but I do think it's a little invasive as far as cluttering up the test directives goes. Open to suggestions for how to possibly improve. Release note: None 29219: kv: truncate RangeFeed span to range descriptor r=nvanbenschoten a=nvanbenschoten Small fix for RangeFeed's interaction with DistSender. Co-authored-by: Justin Jaffray <justin@cockroachlabs.com> Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Build succeeded |
|
backport? could be useful for debugging |
Brought up in #29050 as a possibility to avoid the case of certain rules
no longer applying.
Kind of a strawman right now because I'm not sure
purposes of this PR.
I think this is probably valuable in that it asserts we're testing what
we want to test, but I do think it's a little invasive as far as
cluttering up the test directives goes. Open to suggestions for how to
possibly improve.
Release note: None