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

logictest: improve progress output #72468

Merged
merged 2 commits into from
Jan 27, 2022
Merged

Conversation

andreimatei
Copy link
Contributor

Logictests would print some progress messages to stdout as they run.
Each file prints when it ends and, while it's running, it prints
messages when statements finish successfully (but only up to once per
2s). This patch adds a second-level timestamp to those messages, in
order to provide more clues in the output of timed out logictest
packages. Otherwise, I'm always left guessing if the package had been
making constant progress until the timeout, or if it got stuck at some
point.

Release note: None

@andreimatei andreimatei requested review from otan and a team November 4, 2021 22:12
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg
Copy link
Member

tbg commented Nov 5, 2021

Going to kick this over to @cockroachdb/sql-queries since they're the primary consumers of this format and ultimately need to be on board with the changes.

@tbg tbg requested review from a team and removed request for a team and otan November 5, 2021 00:04
Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei)


pkg/sql/logictest/logic.go, line 1283 at r2 (raw file):

func (t *logicTest) outf(format string, args ...interface{}) {
	if t.verbose {
		finalArgs := make([]interface{}, 1+len(args))

nit: maybe add a quick comment what we're doing with this finalArgs business.

I'm assuming that log.Infof below doesn't need to work with finalArgs because log itself will add a timestamp?

Clarify that the test.progress field refers to statements executed, not
"tests".

Release note: None
Logictests would print some progress messages to stdout as they run.
Each file prints when it ends and, while it's running, it prints
messages when statements finish successfully (but only up to once per
2s). This patch adds a second-level timestamp to those messages, in
order to provide more clues in the output of timed out logictest
packages. Otherwise, I'm always left guessing if the package had been
making constant progress until the timeout, or if it got stuck at some
point.

Release note: None
Copy link
Contributor Author

@andreimatei andreimatei left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)


pkg/sql/logictest/logic.go, line 1283 at r2 (raw file):

nit: maybe add a quick comment what we're doing with this finalArgs business.

please see now

I'm assuming that log.Infof below doesn't need to work with finalArgs because log itself will add a timestamp?

right

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei)

Copy link
Contributor Author

@andreimatei andreimatei left a comment

Choose a reason for hiding this comment

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

bors r+

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei)

@craig
Copy link
Contributor

craig bot commented Jan 26, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jan 26, 2022

This PR was included in a batch that was canceled, it will be automatically retried

@craig
Copy link
Contributor

craig bot commented Jan 26, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jan 27, 2022

Build succeeded:

@craig craig bot merged commit ce6d32a into cockroachdb:master Jan 27, 2022
@andreimatei andreimatei deleted the logictest branch January 29, 2022 21:09
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.

4 participants