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

demo: Add test to verify workload is running with --with-load #40613

Merged
merged 1 commit into from
Sep 24, 2019

Conversation

rohany
Copy link
Contributor

@rohany rohany commented Sep 9, 2019

Adds a test ensuring the workload starts up when running
cockroach demo movr --with-load

Addresses part of #40352.

Release justification: Add tests for an existing feature.

Release note: None

@rohany rohany requested a review from knz September 9, 2019 21:07
@rohany rohany requested a review from a team as a code owner September 9, 2019 21:07
@rohany
Copy link
Contributor Author

rohany commented Sep 9, 2019

@knz I was wondering if you knew a better way to do this, the way I'm doing it seems flaky. I want to continuously send this node statistics query to demo until the result I want comes, or timeout and error in the process. I don't know enough about tcl/expect to do this, so this is a first attempt.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rohany
Copy link
Contributor Author

rohany commented Sep 9, 2019

Ah it looks like your recent PR (#40594) might be helpful.

@rohany
Copy link
Contributor Author

rohany commented Sep 12, 2019

Release justification: Add tests for an existing feature.

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

beware of not making the test run for too long in the "happy" case see comment below

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rohany)


pkg/cli/interactive_tests/test_demo_workload.tcl, line 21 at r1 (raw file):

}

eexpect "SELECT city, id FROM vehicles WHERE city = \$1"

put the expect in the loop so that you break out as soon as it's there (otherwise you're incurring a 10 second mandatory cost to every run of this test)

@rohany
Copy link
Contributor Author

rohany commented Sep 17, 2019

I don't think that is correct -- then we will be expecting 10 times + blocking execution of the loop until the expect statement is satisfied.

@knz
Copy link
Contributor

knz commented Sep 17, 2019

I don't think that is correct -- then we will be expecting 10 times + blocking execution of the loop until the expect statement is satisfied.

You can expect "either something, or nothing after 1 second"
See test_reconnect.tcl for an example.

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

:lgtm: thank you!

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @rohany)

exit 1
}

interrupt
Copy link
Contributor

Choose a reason for hiding this comment

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

you're missing eexpect eof at the end though.

Adds a test ensuring the workload starts up when running
`cockroach demo movr --with-load`

Release justification: Low risk test for existing feature.

Release note: None
@rohany
Copy link
Contributor Author

rohany commented Sep 24, 2019

TFTR! Fixed that.

bors r=knz

craig bot pushed a commit that referenced this pull request Sep 24, 2019
40613: demo: Add test to verify workload is running with --with-load r=knz a=rohany

Adds a test ensuring the workload starts up when running
`cockroach demo movr --with-load`

Addresses part of #40352.

Release justification: Add tests for an existing feature.

Release note: None

Co-authored-by: Rohan Yadav <[email protected]>
@craig
Copy link
Contributor

craig bot commented Sep 24, 2019

Build succeeded

@craig craig bot merged commit 88f4c2f into cockroachdb:master Sep 24, 2019
@rohany
Copy link
Contributor Author

rohany commented Sep 24, 2019

it seems like this is failing on master... might want to revert this and think of a better way to test.

@rohany
Copy link
Contributor Author

rohany commented Sep 24, 2019

actually, i took a look at the logs -- https://teamcity.cockroachdb.com/repository/download/Cockroach_UnitTests/1504667:id/acceptance/TestDockerCLI/test_demo_workload.tcl/runMode%3Ddocker/eb8ba3cc/logs/console-output.log

I think we should disable the license acquisition check for this test then. @knz does that sound like the correct thing to do?

@knz
Copy link
Contributor

knz commented Sep 26, 2019

yes, and thanks for doing it

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