Upgrade Tests: Addressing review comments

bindings/c/test/apitester/TesterApiWorkload.h

@@ -42,7 +42,7 @@ public:
 	virtual void checkProgress() override;
 
 	// Running specific tests
-	// The default implementation generates a workload constisting of
+	// The default implementation generates a workload consisting of
 	// random operations generated by randomOperation
 	virtual void runTests();
 
@@ -81,7 +81,7 @@ protected:
 	int numRandomOperations;
 
 	// The number of transactions to be completed for
-	// a successfull test progress check
+	// a successful test progress check
 	int numOperationsForProgressCheck;
 
 	// Stop command received (for runUntilStop=true)
@@ -93,9 +93,6 @@ protected:
 	// Number of random operations left (for runUntilStop=false)
 	std::atomic<int> numRandomOpLeft;
 
-	// Number of random operations left (for runUntilStop=false)
-	int numProgresCheckOpLeft;
-
 	// Key prefix
 	std::string keyPrefix;
 

bindings/c/test/apitester/TesterOptions.h

@@ -38,6 +38,7 @@ public:
 	std::string logGroup;
 	std::string externalClientLibrary;
 	std::string externalClientDir;
+	bool disableLocalClient = false;
 	std::string testFile;
 	std::string inputPipeName;
 	std::string outputPipeName;

bindings/c/test/apitester/TesterTransactionExecutor.cpp

@@ -221,8 +221,8 @@ protected:
 		err = f.getError();
 		auto waitTimeUs = timeElapsedInUs(start);
 		if (waitTimeUs > LONG_WAIT_TIME_US) {
-			fmt::print("Long waiting time on a future: {}us, return code {} ({}), commit called: {}\n",
-			           waitTimeUs,
+			fmt::print("Long waiting time on a future: {:.3f}s, return code {} ({}), commit called: {}\n",
+			           microsecToSec(waitTimeUs),
 			           err,
 			           fdb_get_error(err),
 			           commitCalled);
@@ -259,9 +259,9 @@ protected:
 		auto waitTimeUs = timeElapsedInUs(start);
 		if (waitTimeUs > LONG_WAIT_TIME_US) {
 			fdb_error_t err3 = onErrorFuture.getError();
-			fmt::print("Long waiting time on onError({}) future: {}us, return code {} ({})\n",
+			fmt::print("Long waiting time on onError({}) future: {:.3f}s, return code {} ({})\n",
 			           err,
-			           waitTimeUs,
+			           microsecToSec(waitTimeUs),
 			           err3,
 			           fdb_get_error(err3));
 		}
@@ -321,8 +321,10 @@ protected:
 		fdb_error_t err = fdb_future_get_error(f);
 		auto waitTimeUs = timeElapsedInUs(cbInfo.startTime, endTime);
 		if (waitTimeUs > LONG_WAIT_TIME_US) {
-			fmt::print(
-			    "Long waiting time on a future: {}us, return code {} ({})\n", waitTimeUs, err, fdb_get_error(err));
+			fmt::print("Long waiting time on a future: {:.3f}s, return code {} ({})\n",
+			           microsecToSec(waitTimeUs),
+			           err,
+			           fdb_get_error(err));
 		}
 		if (err == error_code_transaction_cancelled) {
 			return;
@@ -364,9 +366,9 @@ protected:
 		auto waitTimeUs = timeElapsedInUs(onErrorCallTimePoint);
 		if (waitTimeUs > LONG_WAIT_TIME_US) {
 			fdb_error_t err = onErrorFuture.getError();
-			fmt::print("Long waiting time on onError({}): {}us, return code {} ({})\n",
+			fmt::print("Long waiting time on onError({}): {:.3f}s, return code {} ({})\n",
 			           onErrorArg,
-			           waitTimeUs,
+			           microsecToSec(waitTimeUs),
 			           err,
 			           fdb_get_error(err));
 		}

bindings/c/test/apitester/TesterUtil.cpp

@@ -26,7 +26,7 @@
 
 namespace FdbApiTester {
 
-std::string lower_case(const std::string& str) {
+std::string lowerCase(const std::string& str) {
 	std::string res = str;
 	std::transform(res.begin(), res.end(), res.begin(), ::tolower);
 	return res;

bindings/c/test/apitester/TesterUtil.h

@@ -49,7 +49,7 @@ struct formatter<std::optional<T>> : fmt::formatter<T> {
 
 namespace FdbApiTester {
 
-std::string lower_case(const std::string& str);
+std::string lowerCase(const std::string& str);
 
 class Random {
 public:
@@ -101,6 +101,10 @@ static inline TimeDuration timeElapsedInUs(const TimePoint& start) {
 	return timeElapsedInUs(start, timeNow());
 }
 
+static inline double microsecToSec(TimeDuration timeUs) {
+	return timeUs / 1000000.0;
+}
+
 } // namespace FdbApiTester
 
 #endif

bindings/c/test/apitester/TesterWorkload.cpp

@@ -66,7 +66,7 @@ bool WorkloadConfig::getBoolOption(const std::string& name, bool defaultVal) con
 	if (iter == options.end()) {
 		return defaultVal;
 	} else {
-		std::string val = lower_case(iter->second);
+		std::string val = lowerCase(iter->second);
 		if (val == "true") {
 			return true;
 		} else if (val == "false") {
@@ -280,7 +280,7 @@ void WorkloadManager::confirmProgress(IWorkload* workload) {
 	}
 	lock.unlock();
 	if (allConfirmed) {
-		// Notify the test controller about the successfull progress check
+		// Notify the test controller about the successful progress check
 		ASSERT(outputPipe.is_open());
 		outputPipe << "CHECK_OK" << std::endl;
 	}

bindings/c/test/apitester/TesterWorkload.h

@@ -41,7 +41,7 @@ public:
 	// Stop the workload
 	virtual void stop() = 0;
 
-	// Check and if the test is progressing from the point of calling
+	// Check if the test is progressing from the point of calling
 	// Progress must be confirmed by calling confirmProgress on the workload manager
 	virtual void checkProgress() = 0;
 };
@@ -159,7 +159,7 @@ public:
 	WorkloadManager(ITransactionExecutor* txExecutor, IScheduler* scheduler)
 	  : txExecutor(txExecutor), scheduler(scheduler), numWorkloadsFailed(0) {}
 
-	// Open names pipes for communication with the test controller
+	// Open named pipes for communication with the test controller
 	void openControlPipes(const std::string& inputPipeName, const std::string& outputPipeName);
 
 	// Add a workload
@@ -193,10 +193,10 @@ private:
 	// To be called by a workload to notify that it is done
 	void workloadDone(IWorkload* workload, bool failed);
 
-	// To be called by a workload to confirm a successfull progress check
+	// To be called by a workload to confirm a successful progress check
 	void confirmProgress(IWorkload* workload);
 
-	// Receive and handing control commands from the input pipe
+	// Receive and handle control commands from the input pipe
 	void readControlInput(std::string pipeName);
 
 	// Handle STOP command received from the test controller
@@ -223,7 +223,7 @@ private:
 	// Thread for receiving test control commands
 	std::thread ctrlInputThread;
 
-	// Output pipe for emmitting test control events
+	// Output pipe for emitting test control events
 	std::ofstream outputPipe;
 };
 

bindings/c/test/apitester/fdb_c_api_tester.cpp

@@ -46,6 +46,7 @@ enum TesterOptionId {
 	OPT_KNOB,
 	OPT_EXTERNAL_CLIENT_LIBRARY,
 	OPT_EXTERNAL_CLIENT_DIRECTORY,
+	OPT_DISABLE_LOCAL_CLIENT,
 	OPT_TEST_FILE,
 	OPT_INPUT_PIPE,
 	OPT_OUTPUT_PIPE,
@@ -64,6 +65,7 @@ CSimpleOpt::SOption TesterOptionDefs[] = //
 	  { OPT_KNOB, "--knob-", SO_REQ_SEP },
 	  { OPT_EXTERNAL_CLIENT_LIBRARY, "--external-client-library", SO_REQ_SEP },
 	  { OPT_EXTERNAL_CLIENT_DIRECTORY, "--external-client-dir", SO_REQ_SEP },
+	  { OPT_DISABLE_LOCAL_CLIENT, "--disable-local-client", SO_NONE },
 	  { OPT_TEST_FILE, "-f", SO_REQ_SEP },
 	  { OPT_TEST_FILE, "--test-file", SO_REQ_SEP },
 	  { OPT_INPUT_PIPE, "--input-pipe", SO_REQ_SEP },
@@ -94,6 +96,8 @@ void printProgramUsage(const char* execName) {
 	       "                 Path to the external client library.\n"
 	       "  --external-client-dir DIR\n"
 	       "                 Directory containing external client libraries.\n"
+	       "  --disable-local-client DIR\n"
+	       "                 Disable the local client, i.e. use only external client libraries.\n"
 	       "  --input-pipe NAME\n"
 	       "                 Name of the input pipe for communication with the test controller.\n"
 	       "  --output-pipe NAME\n"
@@ -173,6 +177,9 @@ bool processArg(TesterOptions& options, const CSimpleOpt& args) {
 	case OPT_EXTERNAL_CLIENT_DIRECTORY:
 		options.externalClientDir = args.OptionArg();
 		break;
+	case OPT_DISABLE_LOCAL_CLIENT:
+		options.disableLocalClient = true;
+		break;
 	case OPT_TEST_FILE:
 		options.testFile = args.OptionArg();
 		options.testSpec = readTomlTestSpec(options.testFile);
@@ -227,9 +234,15 @@ void applyNetworkOptions(TesterOptions& options) {
 		fdb_check(
 		    FdbApi::setOption(FDBNetworkOption::FDB_NET_OPTION_EXTERNAL_CLIENT_LIBRARY, options.externalClientLibrary));
 	} else if (!options.externalClientDir.empty()) {
-		fdb_check(FdbApi::setOption(FDBNetworkOption::FDB_NET_OPTION_DISABLE_LOCAL_CLIENT));
+		if (options.disableLocalClient) {
+			fdb_check(FdbApi::setOption(FDBNetworkOption::FDB_NET_OPTION_DISABLE_LOCAL_CLIENT));
+		}
 		fdb_check(
 		    FdbApi::setOption(FDBNetworkOption::FDB_NET_OPTION_EXTERNAL_CLIENT_DIRECTORY, options.externalClientDir));
+	} else {
+		if (options.disableLocalClient) {
+			throw TesterError("Invalid options: Cannot disable local client if no external library is provided");
+		}
 	}
 
 	if (options.testSpec.multiThreaded) {

bindings/c/test/apitester/tests/upgrade/MixedApiWorkloadMultiThr.toml

@@ -15,21 +15,21 @@ maxClients = 8
     [[test.workload]]
     name = 'ApiCorrectness'
     minKeyLength = 1
-	maxKeyLength = 64
-	minValueLength = 1
-	maxValueLength = 1000
-	maxKeysPerTransaction = 50
-	initialSize = 100
-	runUntilStop = true
-	readExistingKeysRatio = 0.9
+    maxKeyLength = 64
+    minValueLength = 1
+    maxValueLength = 1000
+    maxKeysPerTransaction = 50
+    initialSize = 100
+    runUntilStop = true
+    readExistingKeysRatio = 0.9
 
-	[[test.workload]]
+    [[test.workload]]
     name = 'CancelTransaction'
     minKeyLength = 1
-	maxKeyLength = 64
-	minValueLength = 1
-	maxValueLength = 1000
-	maxKeysPerTransaction = 50
-	initialSize = 100
-	runUntilStop = true
-	readExistingKeysRatio = 0.9
+    maxKeyLength = 64
+    minValueLength = 1
+    maxValueLength = 1000
+    maxKeysPerTransaction = 50
+    initialSize = 100
+    runUntilStop = true
+    readExistingKeysRatio = 0.9

bindings/c/test/apitester/tests/upgrade/MixedApiWorkloadSingleThr.toml

@@ -13,21 +13,21 @@ maxClients = 8
     [[test.workload]]
     name = 'ApiCorrectness'
     minKeyLength = 1
-	maxKeyLength = 64
-	minValueLength = 1
-	maxValueLength = 1000
-	maxKeysPerTransaction = 50
-	initialSize = 100
-	runUntilStop = true
-	readExistingKeysRatio = 0.9
+    maxKeyLength = 64
+    minValueLength = 1
+    maxValueLength = 1000
+    maxKeysPerTransaction = 50
+    initialSize = 100
+    runUntilStop = true
+    readExistingKeysRatio = 0.9
 
-	[[test.workload]]
+    [[test.workload]]
     name = 'CancelTransaction'
     minKeyLength = 1
-	maxKeyLength = 64
-	minValueLength = 1
-	maxValueLength = 1000
-	maxKeysPerTransaction = 50
-	initialSize = 100
-	runUntilStop = true
-	readExistingKeysRatio = 0.9
+    maxKeyLength = 64
+    minValueLength = 1
+    maxValueLength = 1000
+    maxKeysPerTransaction = 50
+    initialSize = 100
+    runUntilStop = true
+    readExistingKeysRatio = 0.9

tests/TestRunner/upgrade_test.py

@@ -238,6 +238,7 @@ class UpgradeTest:
                         '--cluster-file', self.cluster.cluster_file,
                         '--test-file', test_file,
                         '--external-client-dir', self.external_lib_dir,
+                        '--disable-local-client',
                         '--input-pipe', self.input_pipe_path,
                         '--output-pipe', self.output_pipe_path,
                         '--api-version', str(self.api_version),