-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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: run four nodes in acceptance tests #17324
Conversation
@@ -268,8 +269,9 @@ upload-coverage: $(BOOTSTRAP_TARGET) | |||
@build/upload-coverage.sh | |||
|
|||
.PHONY: acceptance | |||
acceptance: TESTTIMEOUT := $(ACCEPTANCETIMEOUT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export TESTTIMEOUT := $(ACCEPTANCETIMEOUT)
should do the same without the explicit env
below!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -10,4 +10,4 @@ set -euxo pipefail | |||
mkdir -p artifacts/acceptance | |||
export TMPDIR=$PWD/artifacts/acceptance | |||
|
|||
make test PKG=./pkg/acceptance TAGS=acceptance TESTFLAGS="${TESTFLAGS--v -nodes 3} -l $TMPDIR" | |||
make test PKG=./pkg/acceptance TESTTIMEOUT="${TESTTIMEOUT-30m}" TAGS=acceptance TESTFLAGS="${TESTFLAGS--v -nodes 4} -l $TMPDIR" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be ${TESTTIMEOUT:-30m}
(with the :
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you care about the case in which the variable is set but null? Anecdotally we're using -
over :-
in most places.
This is required for cockroachdb#17272 and merits a separate review.
This is required for
#17272
and merits a separate review.