Solved some warnings for Python PEP 8 convention
Method Names and Instance Variables lowercase with words separated by underscores as necessary to improve readability.
testDir to test_dirinputData to input_dataBlank Lines Surround top-level function and class definitions with two blank lines.
and added verification logic about referenced before assignment for output_type
75 | are not as expected. Error is caught by bctester() and reported. 76 | """ 77 | # Get the exec names and arguments 78 | - execprog = buildenv["BUILDDIR"] + "/src/" + testObj['exec'] + buildenv["EXEEXT"] 79 | - execargs = testObj['args'] 80 | + execprog = buildenv["BUILDDIR"] + "/src/" + test_obj['exec'] + buildenv["EXEEXT"]
I'd assume those should probably use os.path.join to make it work on other os as well.
Thank you for putting so much effort in your first contribution. However, according to our developer guidelines we can not accept pull requests that solely change formatting and style.
Nonetheless, you are welcome to address my issue with this file instead. Also note that you can change testObj to test_obj, as formatting fixes are allowed when none-style but refactoring changes are made to the affected lines.
@MarcoFalke Thank you for your advice, I just added commit for use os.path.join instead string concatenation
76 | are not as expected. Error is caught by bctester() and reported. 77 | """ 78 | # Get the exec names and arguments 79 | - execprog = buildenv["BUILDDIR"] + "/src/" + testObj['exec'] + buildenv["EXEEXT"] 80 | - execargs = testObj['args'] 81 | + execprog = os.path.join(buildenv["BUILDDIR"], "src", test_obj["exec"], buildenv["EXEEXT"])
The last concatenation is string concatenation.
You can check if the tests pass locally by make check
Thanks I just fixed it, Sorry for my careless mistake
45 | + bctester(os.path.join(config["environment"]["SRCDIR"], "test/util/data"), "bitcoin-util-test.json", 46 | + config["environment"]) 47 | + 48 | 49 | -def bctester(testDir, input_basename, buildenv): 50 | +def bctester(test_dir, input_basename, buildenv):
since you're converting to snake_case, how about build_env
76 | are not as expected. Error is caught by bctester() and reported. 77 | """ 78 | # Get the exec names and arguments 79 | - execprog = buildenv["BUILDDIR"] + "/src/" + testObj['exec'] + buildenv["EXEEXT"] 80 | - execargs = testObj['args'] 81 | + execprog = os.path.join(buildenv["BUILDDIR"], "src", test_obj["exec"]) + buildenv["EXEEXT"]
exec_prog? (and exec_args and exec_run)
utACK. A couple of nits inline and commits should be squashed to a single commit before merge.
26 | - config.read_file(open(os.path.dirname(__file__) + "/../config.ini")) 27 | + config.read_file(open(os.path.join(os.path.dirname(__file__), "../config.ini"))) 28 | 29 | parser = argparse.ArgumentParser(description=__doc__) 30 | - parser.add_argument('-v', '--verbose', action='store_true') 31 | + parser.add_argument("-v", "--verbose", action="store_true")
I don't it is worth the overhead to change apostrophe to quotation mark.
agree (and for short symbol-like strings as in this line, single quotes seem to be most people's preferred style)
41 | # Add the format/level to the logger 42 | logging.basicConfig(format=formatter, level=level) 43 | 44 | - bctester(config["environment"]["SRCDIR"] + "/test/util/data", "bitcoin-util-test.json", config["environment"]) 45 | + bctester(os.path.join(config["environment"]["SRCDIR"], "test/util/data"), "bitcoin-util-test.json", 46 | + config["environment"])
Changing this line solely to split it into two causes difficulties grepping in the future.
... and most of our python files don't adhere to the 79 char max line length (which I think is a good thing!)
66 | - failed_testcases.append(testObj["description"]) 67 | + bctest(test_dir, test_obj, build_env) 68 | + logging.info("PASSED: " + test_obj["description"]) 69 | + except Exception as e: 70 | + logging.info("FAILED: " + test_obj["description"]) 71 | + logging.error("Error %s: %s" % (test_obj["description"], e))
Can you explain why it makes sense to first log to info and then the same to the error level?
119 | - if not outputData: 120 | - logging.error("Output data missing for " + outputFn) 121 | + if not output_data: 122 | + logging.error("Output data missing for " + output_fn) 123 | + raise Exception 124 | + if not output_type:
Generally, I'd prefer if refactoring is separate (at least a separate commit) from changing behavior.
190 | raise Exception 191 | 192 | - if "error_txt" in testObj: 193 | - want_error = testObj["error_txt"] 194 | + if "error_txt" in test_obj: 195 | + want_error = test_obj["error_txt"]
I don't think it makes sense to replace every variable in the whole file by a lower_case name.
Again, I appreciate the effort, but according to the developer notes such refactoring patches can not be accepted.
Do not submit patches solely to modify the style of existing code.
And https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring
This will cause merge conflicts with other open pull requests and consumes precious review time.
Please note that the non-refactoring changes in this pull are much appreciated and I'd like to have them merged cleanly without the formatting changes.
19 | @@ -20,40 +20,44 @@ 20 | import subprocess 21 | import sys 22 | 23 | +
For short util modules like this, and even for most of our test scripts, I don't think 2 lines separating each function is necessary. It makes sense for large projects where you want visual separation between top-level classes, but I think it's unnecessary here.
@dongsam we don't have particularly strict style guidelines for our python code. The only style notes are in https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md.
Since A Foolish Consistency is the Hobgoblin of Little Minds, we don't usually open PRs to adhere to PEP-8 style, unless there's some good reason to do so (eg making bugs less likely). If I'm doing substantial work on a test script, I often start the PR with a tidy-up commit, but even that is sometimes unpopular and puts off reviewers.
If you're looking for your first commit, I think @MarcoFalke is right that removing the style changes from this PR and just having the os.path changes would make this more palatable for reviewers.
Needs rebase
42 | @@ -43,12 +43,11 @@ def main(): 43 | formatter = '%(asctime)s - %(levelname)s - %(message)s' 44 | # Add the format/level to the logger 45 | logging.basicConfig(format=formatter, level=level) 46 | -
nit: why remove this line? Blank lines are used within functions to indicate logical sections.
sorry, that was mistake while rebasing, I'll add again now
You've now added trailing whitespace :(
92 | stdinCfg = subprocess.PIPE
93 |
94 | # Read the expected output data (if there is any)
95 | outputFn = None
96 | outputData = None
97 | + outputType = None
Is this change (and the one at line 107) intentional?
Yes, there was potential risk outputType can be None so I added initial outputType and exception line when outputType is None, if you think it is not needed, i can remove again
Yes, this is a good additional check.
106 | raise
107 | if not outputData:
108 | logging.error("Output data missing for " + outputFn)
109 | raise Exception
110 | + if not outputType:
111 | + logging.error("Output type missing for " + outputFn)
suggestion: change the error text slightly here to clarify what the problem is:
logging.error("Output file %s does not have a file extension" % outputFn)
76 | @@ -77,32 +77,36 @@ def bctest(testDir, testObj, buildenv): 77 | are not as expected. Error is caught by bctester() and reported. 78 | """ 79 | # Get the exec names and arguments 80 | - execprog = buildenv["BUILDDIR"] + "/src/" + testObj['exec'] + buildenv["EXEEXT"] 81 | + execprog = os.path.join(buildenv["BUILDDIR"], "src", testObj["exec"]) + buildenv["EXEEXT"]
I think this is better as:
execprog = os.path.join(buildenv["BUILDDIR"], "src", testObj["exec"] + buildenv["EXEEXT"])
(ie buildenv["EXEEXT"] should be appended to testObj["exec"] before joining into a path), but I don't have a windows machine to test this on.
Is there a reason you put buildenv["EXEEXT"] outside the os.path.join?
I thought also but that was mistook, buildenv["EXEEXT"] is not path, so occur error when with os.path.join that need just string concatenation
right, that's why you should have:
execprog = os.path.join(buildenv["BUILDDIR"], "src", testObj["exec"] + buildenv["EXEEXT"])
(concatenate testObj["exec"] and buildenv["EXEEXT"] as a single argument to os.path.join())
oh, sorry I misunderstood, that's look better, I'll update :)
utACK a3ac7672ed9c63b954d3f368a90616448d31b4c3. Needs to be tested on windows.
47 | @@ -48,7 +48,7 @@ def main():
48 |
^ one line above.
"test/util/data" throws an error on windows:
FileNotFoundError: [Errno 2] No such file or directory: '.../Downloads/workspace/bitcoin\\test/util/data\\bitcoin-util-test.json'
47 | @@ -48,7 +48,7 @@ def main(): 48 | 49 | def bctester(testDir, input_basename, buildenv): 50 | """ Loads and parses the input file, runs all tests and reports results""" 51 | - input_filename = testDir + "/" + input_basename 52 | + input_filename = os.path.join(testDir, input_basename)
Might as well pass in input_filename to bctester and instead do the path.join in line 47, no?
Going to merge since this is a strict improvement.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
utACK a3ac7672ed9c63b954d3f368a90616448d31b4c3
-----BEGIN PGP SIGNATURE-----
iQIcBAEBCgAGBQJaRyMzAAoJENLqSFDnUoslIfgP/0jDVYpEb4TBa5/gBHwQJWqk
ez+gDdtwFpJteURQoc4sDFzkGq4xFiltUBuZoDspjgOq8eETvn7mgGpd4t71ZwUy
/7t8yIbFFVR94RhOXCRN/8z9u4vfE7068CeiNwmq+9tP6fRGpsfqCKSbY4qaYuR5
c0RtmOds3ZEknAASMcarT0b+iiBv9gN3ryVlRochrWiO92/lRpfWYvkv0HldDZba
+N1SAPju8KfSjcZRvZ6/3vOyfYChgrXh+y67MJLMGZCkct8YllcjXvPYy5Km74wI
oEZm/TBq4aW3FNgDk4EPHWnKVCA/qC73TzaO3V4QnfqEZz5pXxSEbhldQpo7q+Lm
s/Ve9FJdwMtnS1Gk+wrzoFZCioq6DVgoWixmwT0eJYB2bL9IeeBinceAIOAr08Sz
3IgXgBgaxkaLZSYBp8kkkIsn8WYi0JD2tnDVxcTSBaGH1igdKogLWQDXqEd+Mu1D
k+EArOVMWWeX/pgGoEV1u8Ze7mvZSGwxJxHsJtWGdCoXPgrFrZQFpaba4JkrGTq8
d1vg+8v1+kpxSGlpehiX+2sEunrU5c6XwmSny2zAZassBJ0sTySglhwlqmI493fP
3d0h0mOI2OX7SMxfos1psIrAOTQEEikWqMsxLnG/hehAlj7vOit7GFYCRha6kduE
ElsosotNEXWBPPI7crsO
=hB1L
-----END PGP SIGNATURE-----