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

roachtest: add acceptance/bank/{cluster-recovery,node-restart} #29449

Merged
merged 7 commits into from
Sep 1, 2018

Conversation

petermattis
Copy link
Collaborator

Move the cluster-recovery and node-restart acceptance tests to
acceptance/bank/{cluster-recovery,node-restart} roachtests.

See #29151

Release note: None

Move the cluster-recovery and node-restart acceptance tests to
acceptance/bank/{cluster-recovery,node-restart} roachtests.

See cockroachdb#29151

Release note: None
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@petermattis petermattis requested a review from benesch August 31, 2018 21:34
Copy link
Collaborator Author

@petermattis petermattis 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


pkg/cmd/roachtest/bank.go, line 14 at r3 (raw file):

// implied. See the License for the specific language governing
// permissions and limitations under the License. See the AUTHORS file
// for names of contributors.

I copied pasted this from another file. We should probably remove the See the AUTHORS file line everywhere because we don't have one.

Copy link
Collaborator Author

@petermattis petermattis left a comment

Choose a reason for hiding this comment

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

I tried to keep the first commit minimal (though I failed slightly).

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@petermattis petermattis force-pushed the pmattis/roachtest-bank branch from e1ce113 to 0867544 Compare August 31, 2018 23:27
Create a global context that all test contexts are derived from. When an
interrupt signal is received this global context is cancelled which
propagates cancellation to all of the test contexts.

Release note: None
@petermattis petermattis force-pushed the pmattis/roachtest-bank branch from 0867544 to 8624547 Compare September 1, 2018 01:50
For some reason I thought the local cluster size for the legacy
acceptance tests was 3. It is actually 4, so we should use 4 for the
roachtest-based acceptance tests.

Release note: None
Move the status-server acceptance test to a new acceptance/status-server
roachtest.

See cockroachdb#29151

Release note: None
Move the rapid-restart acceptance test to a new acceptance/rapid-restart
roachtest.

See cockroachdb#29151

Release note: None
Copy link
Contributor

@benesch benesch left a comment

Choose a reason for hiding this comment

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

This is exciting cleanup!

Reviewed 2 of 2 files at r1, 1 of 1 files at r2, 3 of 3 files at r4, 1 of 1 files at r5, 3 of 3 files at r6, 4 of 4 files at r7.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/cmd/roachtest/acceptance.go, line 23 at r5 (raw file):

	spec := testSpec{
		Name:  "acceptance",
		Nodes: nodes(4),

Ooh, nice catch.


pkg/cmd/roachtest/bank.go, line 14 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I copied pasted this from another file. We should probably remove the See the AUTHORS file line everywhere because we don't have one.

Yeah, only 109 of the files have this extra sentence, out of 1870 total that use the Apache license. How odd. This is like a disease that is spreading through our license headers.

Copy link
Collaborator Author

@petermattis petermattis left a comment

Choose a reason for hiding this comment

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

TFTR! I should be able to finish the rest in another few hours. As predicted, this is mostly mechanical work.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/cmd/roachtest/acceptance.go, line 23 at r5 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Ooh, nice catch.

Yeah, well one of the "local" acceptance tests I haven't got to requires 4 nodes which made this sort of obvious at that point.


pkg/cmd/roachtest/bank.go, line 14 at r3 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Yeah, only 109 of the files have this extra sentence, out of 1870 total that use the Apache license. How odd. This is like a disease that is spreading through our license headers.

I'll do this in a follow-on PR. Maybe further into the 2.2 cycle.

Centralize wiping of the cluster before running acceptance tests. For
tests which share a cluster, we need to wipe the old state before
putting new binaries.

Fixes cockroachdb#29458

Release note: None
@petermattis petermattis force-pushed the pmattis/roachtest-bank branch from 16efd3e to 86ade65 Compare September 1, 2018 13:16
@petermattis
Copy link
Collaborator Author

bors r=benesch

@craig
Copy link
Contributor

craig bot commented Sep 1, 2018

Build failed

@petermattis
Copy link
Collaborator Author

bors r=benesch

craig bot pushed a commit that referenced this pull request Sep 1, 2018
29449: roachtest: add acceptance/bank/{cluster-recovery,node-restart} r=benesch a=petermattis

Move the cluster-recovery and node-restart acceptance tests to
acceptance/bank/{cluster-recovery,node-restart} roachtests.

See #29151

Release note: None

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

craig bot commented Sep 1, 2018

Build succeeded

@craig craig bot merged commit 86ade65 into cockroachdb:master Sep 1, 2018
@petermattis petermattis deleted the pmattis/roachtest-bank branch September 1, 2018 17:03
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