Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3341024:
Add Python tests for build_image script (Closed)
|
Created:
10 years, 3 months ago by Tan Gao Modified:
9 years, 7 months ago CC:
chromium-os-reviews_chromium.org, Mandeep Singh Baines, anush, sosa Visibility:
Public. |
DescriptionAdd Python tests for build_image script
Change-Id: I95bb4518bbfa2931407a90180686845fd9e928e5
BUG=chromium-os:6574
TEST=manually run test script and verified all 4 test cases passed
Patch Set 1 #
Total comments: 47
Patch Set 2 : refactor/rewrite per review feedback #Patch Set 3 : use new return object from RunCommand #
Total comments: 8
Patch Set 4 : rename functions to gpylint, minor fixes per review #Messages
Total messages: 10 (0 generated)
http://codereview.chromium.org/3341024/diff/1/2 File chromite/tests/build_image_test.py (right): http://codereview.chromium.org/3341024/diff/1/2#newcode33 chromite/tests/build_image_test.py:33: if os.path.exists('/etc/debian_chroot'): Can we use a different file to check if we are inside the chroot? debian_chroot is a bit misleading because we don't use debian anymore http://codereview.chromium.org/3341024/diff/1/2#newcode106 chromite/tests/build_image_test.py:106: cmds.append('./enter_chroot.sh') Could we adjust this to just assume we are inside the chroot? Also, I don't think it's necessary to keep a process open and talk to it interactively -- that sounds like it's overcomplicated. http://codereview.chromium.org/3341024/diff/1/2#newcode107 chromite/tests/build_image_test.py:107: self.p = subprocess.Popen(';'.join(cmds), Can we make a library for doing what you want here? We're probably going to have a bunch of scripts that all run commands so we want to keep the complexity out of the original scripts. http://codereview.chromium.org/3341024/diff/1/2#newcode115 chromite/tests/build_image_test.py:115: # FIXME(tgao): calling terminate() here affects subsequent tests! Hmm. That sounds bad. Let's fix this before we commit. Assuming we are inside the chroot should help with this. http://codereview.chromium.org/3341024/diff/1/2#newcode134 chromite/tests/build_image_test.py:134: def _RunBuildImageCmd(self, cmd, assert_success=False): Could you remove the default here, or set assert_success=True? It seems counterintuitive to default to assuming failure.
http://codereview.chromium.org/3341024/diff/1/2 File chromite/tests/build_image_test.py (right): http://codereview.chromium.org/3341024/diff/1/2#newcode33 chromite/tests/build_image_test.py:33: if os.path.exists('/etc/debian_chroot'): On 2010/09/09 17:17:42, davidjames wrote: > Can we use a different file to check if we are inside the chroot? debian_chroot > is a bit misleading because we don't use debian anymore Not arguing that this should be changed, but the check in common.sh should get updated at the same time. Would be good to keep them in sync (and possibly add a comment in each file referencing the duplicate function) http://codereview.chromium.org/3341024/diff/1/2#newcode182 chromite/tests/build_image_test.py:182: # TODO(tgao): refactor this test case to support all board types If you're already using a variable to store the board name, why not just rename this: testBuidlimageForBoard(self, board='x86_generic') so you don't need to refactor?
http://codereview.chromium.org/3341024/diff/1/2 File chromite/tests/build_image_test.py (right): http://codereview.chromium.org/3341024/diff/1/2#newcode32 chromite/tests/build_image_test.py:32: """Returns True if we are inside chroot.""" You may wanna put this in the common lib as opposed to here. http://codereview.chromium.org/3341024/diff/1/2#newcode33 chromite/tests/build_image_test.py:33: if os.path.exists('/etc/debian_chroot'): Python style prefers using less control statements if possible so for this, just return os.path.exists('/etc/debian_chroot') http://codereview.chromium.org/3341024/diff/1/2#newcode39 chromite/tests/build_image_test.py:39: """Get absolute path to src/scripts/ directory. This is not what it does, this gives you the absolute path to the Chromium OS base dir not the Src dir. http://codereview.chromium.org/3341024/diff/1/2#newcode41 chromite/tests/build_image_test.py:41: Assuming test script will always be run from the same root as src/scripts. Not quite right, just assuming it's some descendent of src/scripts http://codereview.chromium.org/3341024/diff/1/2#newcode48 chromite/tests/build_image_test.py:48: test_script_path = os.path.abspath('.') I think os.cwd is preferred over '.' http://codereview.chromium.org/3341024/diff/1/2#newcode63 chromite/tests/build_image_test.py:63: """Helper method to parse output for CHROMEOS_VERSION_STRING. Put in common. http://codereview.chromium.org/3341024/diff/1/2#newcode66 chromite/tests/build_image_test.py:66: output: a string, stdout returned from child process. Give better name. It's too specific to the call from this script. http://codereview.chromium.org/3341024/diff/1/2#newcode70 chromite/tests/build_image_test.py:70: chromeos_version.sh. Or None if not found. two space indent here http://codereview.chromium.org/3341024/diff/1/2#newcode81 chromite/tests/build_image_test.py:81: def GetOutputImageDir(board, cros_version): common :D http://codereview.chromium.org/3341024/diff/1/2#newcode85 chromite/tests/build_image_test.py:85: board: a string, board type. Don't need to define types, it should be obvious given the rest of the comment. http://codereview.chromium.org/3341024/diff/1/2#newcode91 chromite/tests/build_image_test.py:91: src_root = GetSrcRoot() Need to fix src root, right now gives Chromium OS base dir so the build dir is incorrect. http://codereview.chromium.org/3341024/diff/1/2#newcode95 chromite/tests/build_image_test.py:95: output_dir = os.path.join(os.path.dirname(src_root), rel_path, version_str) Why dirname? Shouldn't it already be a dir name? http://codereview.chromium.org/3341024/diff/1/2#newcode106 chromite/tests/build_image_test.py:106: cmds.append('./enter_chroot.sh') On 2010/09/09 17:17:42, davidjames wrote: > Could we adjust this to just assume we are inside the chroot? Also, I don't > think it's necessary to keep a process open and talk to it interactively -- that > sounds like it's overcomplicated. 100% agreed here. http://codereview.chromium.org/3341024/diff/1/2#newcode125 chromite/tests/build_image_test.py:125: Otherwise use its stderr. 2 space indent for docstring continuations http://codereview.chromium.org/3341024/diff/1/2#newcode142 chromite/tests/build_image_test.py:142: logging.info('About to run command: %s', cmd) IF you're already in the chroot, just use RunCommand http://codereview.chromium.org/3341024/diff/1/2#newcode159 chromite/tests/build_image_test.py:159: expected_images = ['chromiumos_image.bin', 'chromiumos_base_image.bin'] This should belong to the test, not the checking method. Move this def to the test and take it as an argument http://codereview.chromium.org/3341024/diff/1/2#newcode166 chromite/tests/build_image_test.py:166: self._RunBuildImageCmd('./build_image') This will fail if a default board is set. Maybe you want to make sure to delete .default_board or change it to something else and put it back after the test.
Thanks for the thorough review, guys!! I'll start a separate CL to patch RunCommand so it can be used by tests here :-) http://codereview.chromium.org/3341024/diff/1/2 File chromite/tests/build_image_test.py (right): http://codereview.chromium.org/3341024/diff/1/2#newcode32 chromite/tests/build_image_test.py:32: """Returns True if we are inside chroot.""" added TODO (postponing this so I can batch up more methods to be moved from this script :-) http://codereview.chromium.org/3341024/diff/1/2#newcode33 chromite/tests/build_image_test.py:33: if os.path.exists('/etc/debian_chroot'): this is a straight port from common.sh, which uses the same check to see if we're inside chroot http://codereview.chromium.org/3341024/diff/1/2#newcode33 chromite/tests/build_image_test.py:33: if os.path.exists('/etc/debian_chroot'): On 2010/09/09 17:43:24, sosa wrote: > Python style prefers using less control statements if possible so for this, > just return os.path.exists('/etc/debian_chroot') Done. http://codereview.chromium.org/3341024/diff/1/2#newcode33 chromite/tests/build_image_test.py:33: if os.path.exists('/etc/debian_chroot'): they're in sync (Python method here is a straight port from shell :-) http://codereview.chromium.org/3341024/diff/1/2#newcode39 chromite/tests/build_image_test.py:39: """Get absolute path to src/scripts/ directory. "src_root" points to /blah/.../src/scripts. (so that I can invoke "./enter_chroot.sh" :-) http://codereview.chromium.org/3341024/diff/1/2#newcode41 chromite/tests/build_image_test.py:41: Assuming test script will always be run from the same root as src/scripts. revised http://codereview.chromium.org/3341024/diff/1/2#newcode48 chromite/tests/build_image_test.py:48: test_script_path = os.path.abspath('.') On 2010/09/09 17:43:24, sosa wrote: > I think os.cwd is preferred over '.' >>> os.cwd Traceback (most recent call last): File "<stdin>", line 1, in <module> AttributeError: 'module' object has no attribute 'cwd' http://codereview.chromium.org/3341024/diff/1/2#newcode63 chromite/tests/build_image_test.py:63: """Helper method to parse output for CHROMEOS_VERSION_STRING. added TODO http://codereview.chromium.org/3341024/diff/1/2#newcode66 chromite/tests/build_image_test.py:66: output: a string, stdout returned from child process. s/output/s/ http://codereview.chromium.org/3341024/diff/1/2#newcode70 chromite/tests/build_image_test.py:70: chromeos_version.sh. Or None if not found. On 2010/09/09 17:43:24, sosa wrote: > two space indent here Done. http://codereview.chromium.org/3341024/diff/1/2#newcode81 chromite/tests/build_image_test.py:81: def GetOutputImageDir(board, cros_version): On 2010/09/09 17:43:24, sosa wrote: > common :D Done. http://codereview.chromium.org/3341024/diff/1/2#newcode85 chromite/tests/build_image_test.py:85: board: a string, board type. s/, board type// http://codereview.chromium.org/3341024/diff/1/2#newcode91 chromite/tests/build_image_test.py:91: src_root = GetSrcRoot() src_root now points to ".../src/scripts/" (yes, tested :-) http://codereview.chromium.org/3341024/diff/1/2#newcode95 chromite/tests/build_image_test.py:95: output_dir = os.path.join(os.path.dirname(src_root), rel_path, version_str) dirname is used to remove "scripts" from ".../src/scripts" (no ending slash) so we can construct the proper output directory http://codereview.chromium.org/3341024/diff/1/2#newcode106 chromite/tests/build_image_test.py:106: cmds.append('./enter_chroot.sh') eventually, we want to test the case of developer workflow starting from "make_chroot", in that case, can't assume we have a working chroot already. I'll remove these for now but leave a future TODO when we expand the test suite http://codereview.chromium.org/3341024/diff/1/2#newcode107 chromite/tests/build_image_test.py:107: self.p = subprocess.Popen(';'.join(cmds), removed these and will update RunCommand in a separate CL http://codereview.chromium.org/3341024/diff/1/2#newcode115 chromite/tests/build_image_test.py:115: # FIXME(tgao): calling terminate() here affects subsequent tests! removed since our assumption has changed to be inside chroot always http://codereview.chromium.org/3341024/diff/1/2#newcode125 chromite/tests/build_image_test.py:125: Otherwise use its stderr. On 2010/09/09 17:43:24, sosa wrote: > 2 space indent for docstring continuations Done. http://codereview.chromium.org/3341024/diff/1/2#newcode134 chromite/tests/build_image_test.py:134: def _RunBuildImageCmd(self, cmd, assert_success=False): good call, done! http://codereview.chromium.org/3341024/diff/1/2#newcode142 chromite/tests/build_image_test.py:142: logging.info('About to run command: %s', cmd) I'll patch RunCommand to return error as well since I need to parse it http://codereview.chromium.org/3341024/diff/1/2#newcode159 chromite/tests/build_image_test.py:159: expected_images = ['chromiumos_image.bin', 'chromiumos_base_image.bin'] On 2010/09/09 17:43:24, sosa wrote: > This should belong to the test, not the checking method. Move this def to the > test and take it as an argument Done. http://codereview.chromium.org/3341024/diff/1/2#newcode166 chromite/tests/build_image_test.py:166: self._RunBuildImageCmd('./build_image') explicitly setting --board to empty string '' to overwrite .default_board (if present) http://codereview.chromium.org/3341024/diff/1/2#newcode182 chromite/tests/build_image_test.py:182: # TODO(tgao): refactor this test case to support all board types On 2010/09/09 17:35:20, kliegs wrote: > If you're already using a variable to store the board name, why not just rename > this: > testBuidlimageForBoard(self, board='x86_generic') so you don't need to > refactor? Done.
CL updated w/ latest RunCommand change (return an object). PTAL thx!
Could you update this CL to pass gpylint? gpylint requires that functions AreCapitalizedLikeThis. Otherwise, LGTM. Cheers, David
some nits http://codereview.chromium.org/3341024/diff/11001/12001 File chromite/tests/build_image_test.py (right): http://codereview.chromium.org/3341024/diff/11001/12001#newcode34 chromite/tests/build_image_test.py:34: def is_inside_chroot(): Not sure what happened but the style for your method names is incorrect. Should be IsInsideChroot http://codereview.chromium.org/3341024/diff/11001/12001#newcode50 chromite/tests/build_image_test.py:50: logging.debug('test_script_path = %r', test_script_path) %r? http://codereview.chromium.org/3341024/diff/11001/12001#newcode68 chromite/tests/build_image_test.py:68: str_obj: a string. um, string? can you be more specific :D http://codereview.chromium.org/3341024/diff/11001/12001#newcode110 chromite/tests/build_image_test.py:110: def _check_child_process(self, query_list, check_stdout=False): Can you rename to be more consistent with its doing now that you've refactored and aren't monitoring a child process?
http://codereview.chromium.org/3341024/diff/11001/12001 File chromite/tests/build_image_test.py (right): http://codereview.chromium.org/3341024/diff/11001/12001#newcode34 chromite/tests/build_image_test.py:34: def is_inside_chroot(): not an accident.. I followed pylint rules and changed everything manually .. uhh .. undo those changes again :-) http://codereview.chromium.org/3341024/diff/11001/12001#newcode50 chromite/tests/build_image_test.py:50: logging.debug('test_script_path = %r', test_script_path) I prefer %r over %s since it's more informative (e.g. in case test_script_path is not a string, %r will show what it actually is :-) On 2010/09/10 22:57:18, sosa wrote: > %r? http://codereview.chromium.org/3341024/diff/11001/12001#newcode68 chromite/tests/build_image_test.py:68: str_obj: a string. On 2010/09/10 22:57:18, sosa wrote: > um, string? can you be more specific :D Done. http://codereview.chromium.org/3341024/diff/11001/12001#newcode110 chromite/tests/build_image_test.py:110: def _check_child_process(self, query_list, check_stdout=False): _CheckStringPresent is the best I could think of :-)
lgtm! |
