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

build: move {metacheck,unconvert} into style_test #19395

Merged
merged 5 commits into from
Oct 21, 2017
Merged

build: move {metacheck,unconvert} into style_test #19395

merged 5 commits into from
Oct 21, 2017

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Oct 19, 2017

Avoids shelling out; runs everything in the test process and avoids unconvert's extra parsing of the AST.

@tamird tamird requested review from nvanbenschoten and a team October 19, 2017 23:12
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tamird tamird requested a review from a team as a code owner October 20, 2017 00:44
@tamird tamird requested review from a team October 20, 2017 00:44
@tamird tamird requested review from dt and tbg October 20, 2017 10:05
@@ -76,7 +76,7 @@ start_test "Test that quit does not emit unwanted logging output"
# Unwanted: between the point the command starts until it
# either prints the final ok message or fails with some error
# (e.g. due to no definite answer from the server).
send "echo marker; $argv quit 2>&1 | grep '^\\(\[IWEF\]\[0-9\]\\)' \r"
send "echo marker; $argv quit 2>&1 | grep -vE '^\[IWEF\]\[0-9\]+ grpc: '\r"
Copy link
Member

Choose a reason for hiding this comment

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

Not good enough, I think! We need to match

W171020 12:24:14.884065 21 vendor/google.golang.org/grpc/clientconn.go:1034 grpc: addrConn.resetTransport failed to create client transport:

So I'm missing a .* somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Fixed in #19402.

@tamird
Copy link
Contributor Author

tamird commented Oct 20, 2017

In my testing, this reduces make lint from 86s to 56s. (CI failed on an acceptance timeout that seems unrelated to this PR). make lintshort is unaffected.

@tamird
Copy link
Contributor Author

tamird commented Oct 20, 2017

Ah, looks like @a-robinson kindly rebuilt this.

@tbg
Copy link
Member

tbg commented Oct 20, 2017

This looks pretty good to me and the speedup is nice. It could use a few more inline comments (i.e. what's the megachecker struct, etc) but :lgtm: and thanks!


Reviewed 1 of 1 files at r1, 28 of 28 files at r6, 3 of 3 files at r7, 1 of 1 files at r8, 26 of 26 files at r9, 1 of 1 files at r10.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@tamird
Copy link
Contributor Author

tamird commented Oct 21, 2017

Happy to add whatever comments you'd like to see; added one for the struct, but lmk if you want to see others - I'll do that in a follow-up.

Gonna need a rebuild to get past these new flakes though!

@tamird
Copy link
Contributor Author

tamird commented Oct 21, 2017

Thank you, kind stranger!

@tamird tamird merged commit 2c9a884 into cockroachdb:master Oct 21, 2017
@tamird tamird deleted the megacheck branch October 21, 2017 02: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