From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-27.smtp.github.com (out-27.smtp.github.com [192.30.252.210]) by mail.toke.dk (Postfix) with ESMTPS id 50DA68D434A for ; Wed, 20 Oct 2021 23:20:55 +0200 (CEST) Authentication-Results: mail.toke.dk; dkim=pass (1024-bit key) header.d=github.com header.i=@github.com header.b=eHxkUC8Y Received: from github-lowworker-bc343b9.ash1-iad.github.net (github-lowworker-bc343b9.ash1-iad.github.net [10.56.122.73]) by smtp.github.com (Postfix) with ESMTP id A8AEB900390 for ; Wed, 20 Oct 2021 14:20:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=github.com; s=pf2014; t=1634764854; bh=ijky7nSsmLOypk1Z7tvNY/EIT63rIZZM+iaqLduLEz0=; h=Date:From:Reply-To:To:Cc:In-Reply-To:References:Subject:List-ID: List-Archive:List-Post:List-Unsubscribe:From; b=eHxkUC8YItbb5xzqLk37zyYNRklPsoZSdhiiBBxM8ZI+QiA14L5lU4759ByLFBHgo vHV6a81sNqOPrmFyW+BGhn3hQj/GKQblrGDETo6OxYxUR32X9pVoDIAcYwh2f3lAWE qc5xOnwUiP/uZRp/alNmrahUxHpMJP/mRmjExsEQ= Date: Wed, 20 Oct 2021 14:20:54 -0700 From: =?UTF-8?B?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2Vu?= To: tohojo/flent Message-ID: In-Reply-To: References: Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="--==_mimepart_617088369a80c_315ac7101597fe"; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: list X-GitHub-Sender: tohojo X-GitHub-Recipient: flent-users X-GitHub-Reason: subscribed X-Auto-Response-Suppress: All X-GitHub-Recipient-Address: flent-users@flent.org Message-ID-Hash: FAET35VOXNSGJ24PF4IS7PXRF3PTNZK6 X-Message-ID-Hash: FAET35VOXNSGJ24PF4IS7PXRF3PTNZK6 X-MailFrom: noreply@github.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Subscribed X-Mailman-Version: 3.3.4 Reply-To: tohojo/flent Subject: [Flent-users] Re: [tohojo/flent] Added --test-payload option (PR #243) List-Id: Flent discussion list Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: ----==_mimepart_617088369a80c_315ac7101597fe Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit @tohojo requested changes on this pull request. That's better! One more small nit below. Also, the commit message you added is great, but please squash the two commits together so that it becomes one commit that keeps the heading of the first one, and has the text of the second commit as its body. Also, this sentence: > Using the option will cause netperf to fail noisily, in case the specified file is not readable or does not exist. is slightly misleading. It's not netperf that fails noisily, it's the check in flent. So maybe "Flent will check if the file is readable before passing it to netperf, and fail the test run if it isn't" ? > except RunnerCheckError: - netperf['buffer'] = '' + if(fill_file == '/dev/urandom'): + netperf['buffer'] = '' + else: + # If the custom file is not readable, fail noisily + raise RunnerCheckError("The specified fill file does not exist or is not readable.") The option is --test-payload, so referring to "fill file" in the error message is going to confuse people. Better stick to "test payload" here as well :) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/tohojo/flent/pull/243#pullrequestreview-785005703 ----==_mimepart_617088369a80c_315ac7101597fe Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit

@tohojo requested changes on this pull request.

That's better! One more small nit below. Also, the commit message you added is great, but please squash the two commits together so that it becomes one commit that keeps the heading of the first one, and has the text of the second commit as its body.

Also, this sentence:

Using the option will cause netperf to fail noisily, in case the specified file is not readable or does not exist.

is slightly misleading. It's not netperf that fails noisily, it's the check in flent. So maybe "Flent will check if the file is readable before passing it to netperf, and fail the test run if it isn't" ?

In flent/runners.py:

>              except RunnerCheckError:
-                netperf['buffer'] = ''
+                if(fill_file == '/dev/urandom'):
+                    netperf['buffer'] = ''
+                else:
+                    # If the custom file is not readable, fail noisily
+                    raise RunnerCheckError("The specified fill file does not exist or is not readable.")

The option is --test-payload, so referring to "fill file" in the error message is going to confuse people. Better stick to "test payload" here as well :)

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.

----==_mimepart_617088369a80c_315ac7101597fe--