Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7671049:
Refactor/Simplify the test expectation code in media/tools/layout_tests/ (Closed)
|
Created:
9 years, 4 months ago by imasaki1 Modified:
9 years, 4 months ago CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, Paweł Hajdan Jr., acolwell+watch_chromium.org, annacc+watch_chromium.org, pam+watch_chromium.org, ajwong+watch_chromium.org, vrk (LEFT CHROMIUM), scherkus (not reviewing) Visibility:
Public. |
DescriptionRefactor/Simplify the test expectation code in media/tools/layout_tests/, I will first try to checkin minimum working code and expand it from there.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97704
Patch Set 1 #
Total comments: 16
Patch Set 2 : Modification based on CR comments. #Patch Set 3 : Modification based on CR comments. #
Total comments: 22
Patch Set 4 : Modification based on CR comments. #Patch Set 5 : Modified based on CR comments. #
Total comments: 2
Patch Set 6 : Modification based on CR comments. #Patch Set 7 : adding blank lines. #
Total comments: 64
Patch Set 8 : Modification based on CR comments by Dennis. #Patch Set 9 : Remove 'foo' from one of the unit test name. #Patch Set 10 : Minor name change. #
Total comments: 5
Patch Set 11 : Modification based on CR comments. #
Messages
Total messages: 20 (0 generated)
Thanks.
http://codereview.chromium.org/7671049/diff/1/media/tools/layout_tests/test_e... File media/tools/layout_tests/test_expectations.py (right): http://codereview.chromium.org/7671049/diff/1/media/tools/layout_tests/test_e... media/tools/layout_tests/test_expectations.py:12: # TODO(imasaki@) : support multiple test expectations files. No '@' on TODO. http://codereview.chromium.org/7671049/diff/1/media/tools/layout_tests/test_e... media/tools/layout_tests/test_expectations.py:37: http://svn.webkit.org/repository/webkit/trunk/LayoutTests/platform/ Can reference DEFAULT above http://codereview.chromium.org/7671049/diff/1/media/tools/layout_tests/test_e... media/tools/layout_tests/test_expectations.py:40: the analysis (joining with exisinting layout tests using a test name). typo: existinting http://codereview.chromium.org/7671049/diff/1/media/tools/layout_tests/test_e... media/tools/layout_tests/test_expectations.py:41: Instance variable |all_test_expectation_info| is ussed. ussed http://codereview.chromium.org/7671049/diff/1/media/tools/layout_tests/test_e... media/tools/layout_tests/test_expectations.py:49: In this case, all_test_expectation_info['media/video-zoom.html'] will have s/have/have a/ http://codereview.chromium.org/7671049/diff/1/media/tools/layout_tests/test_e... media/tools/layout_tests/test_expectations.py:50: list with two element, each of which is the map of the entries' s/element/elements/ http://codereview.chromium.org/7671049/diff/1/media/tools/layout_tests/test_e... media/tools/layout_tests/test_expectations.py:50: list with two element, each of which is the map of the entries' entries' what? http://codereview.chromium.org/7671049/diff/1/media/tools/layout_tests/test_e... media/tools/layout_tests/test_expectations.py:53: @staticmethod Stopped reviewing at this point b/c I realized this is a parser for test_expectations.txt, and surely we already have something for that. Indeed, http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/To... is exactly that. An example of using it from chromium python code (in case you want to avoid upstreaming the analyzer to webkit) is http://src.chromium.org/viewvc/chrome/trunk/src/webkit/tools/layout_tests/web... Will that allow you to simply delete this file and its test?
Thank you. http://codereview.chromium.org/7671049/diff/1/media/tools/layout_tests/test_e... File media/tools/layout_tests/test_expectations.py (right): http://codereview.chromium.org/7671049/diff/1/media/tools/layout_tests/test_e... media/tools/layout_tests/test_expectations.py:12: # TODO(imasaki@) : support multiple test expectations files. On 2011/08/18 19:56:58, Ami Fischman wrote: > No '@' on TODO. Done. http://codereview.chromium.org/7671049/diff/1/media/tools/layout_tests/test_e... media/tools/layout_tests/test_expectations.py:37: http://svn.webkit.org/repository/webkit/trunk/LayoutTests/platform/ On 2011/08/18 19:56:58, Ami Fischman wrote: > Can reference DEFAULT above Done. http://codereview.chromium.org/7671049/diff/1/media/tools/layout_tests/test_e... media/tools/layout_tests/test_expectations.py:40: the analysis (joining with exisinting layout tests using a test name). On 2011/08/18 19:56:58, Ami Fischman wrote: > typo: existinting Done. http://codereview.chromium.org/7671049/diff/1/media/tools/layout_tests/test_e... media/tools/layout_tests/test_expectations.py:41: Instance variable |all_test_expectation_info| is ussed. On 2011/08/18 19:56:58, Ami Fischman wrote: > ussed Done. http://codereview.chromium.org/7671049/diff/1/media/tools/layout_tests/test_e... media/tools/layout_tests/test_expectations.py:49: In this case, all_test_expectation_info['media/video-zoom.html'] will have On 2011/08/18 19:56:58, Ami Fischman wrote: > s/have/have a/ Done. http://codereview.chromium.org/7671049/diff/1/media/tools/layout_tests/test_e... media/tools/layout_tests/test_expectations.py:50: list with two element, each of which is the map of the entries' On 2011/08/18 19:56:58, Ami Fischman wrote: > s/element/elements/ Done. http://codereview.chromium.org/7671049/diff/1/media/tools/layout_tests/test_e... media/tools/layout_tests/test_expectations.py:53: @staticmethod On 2011/08/18 19:56:58, Ami Fischman wrote: > Stopped reviewing at this point b/c I realized this is a parser for > test_expectations.txt, and surely we already have something for that. > > Indeed, > http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/To... > is exactly that. > An example of using it from chromium python code (in case you want to avoid > upstreaming the analyzer to webkit) is > http://src.chromium.org/viewvc/chrome/trunk/src/webkit/tools/layout_tests/web... This class is broken since test_expectations.TestExpectationsFile is deleted by the change http://trac.webkit.org/changeset/92861. Also, there is class location change. > > Will that allow you to simply delete this file and its test? I have been trying on this for 3 hours or so. However, it is very difficult to use their package from chromium script. I think for now. I would like to keep as it is. Let me know what you think.
http://codereview.chromium.org/7671049/diff/1/media/tools/layout_tests/test_e... File media/tools/layout_tests/test_expectations.py (right): http://codereview.chromium.org/7671049/diff/1/media/tools/layout_tests/test_e... media/tools/layout_tests/test_expectations.py:53: @staticmethod On 2011/08/18 22:51:56, imasaki1 wrote: > On 2011/08/18 19:56:58, Ami Fischman wrote: > > Stopped reviewing at this point b/c I realized this is a parser for > > test_expectations.txt, and surely we already have something for that. > > > > Indeed, > > > http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/To... > > is exactly that. > > An example of using it from chromium python code (in case you want to avoid > > upstreaming the analyzer to webkit) is > > > http://src.chromium.org/viewvc/chrome/trunk/src/webkit/tools/layout_tests/web... > > This class is broken since test_expectations.TestExpectationsFile is deleted by > the change http://trac.webkit.org/changeset/92861. Also, there is class location > change. I think you meant http://trac.webkit.org/changeset/90934 But that CL's description makes it seem like TestExpectationsFile was not needed in the first place. I wasn't suggesting you *use* expectations_line.py, only that it might serve as a useful example for you for how to use test_expectations.py > > Will that allow you to simply delete this file and its test? > I have been trying on this for 3 hours or so. However, it is very difficult to > use their package from chromium script. I think for now. I would like to keep > as it is. Let me know what you think. What I think is that to do the job right here, you will end up with a copy of the webkit module, anyway. For example, in the webkit module I see AUDIO and NOW, which don't appear in this file. I don't know if that's a bug here or there or in the test_expectations.txt file's comments, but ISTM that having this copy that nobody else knows about is nearly guaranteed to go stale very quickly. What difficulty did you run into using the webkit module from a chromium script?
On 2011/08/19 15:43:00, Ami Fischman wrote: > http://codereview.chromium.org/7671049/diff/1/media/tools/layout_tests/test_e... > File media/tools/layout_tests/test_expectations.py (right): > > http://codereview.chromium.org/7671049/diff/1/media/tools/layout_tests/test_e... > media/tools/layout_tests/test_expectations.py:53: @staticmethod > On 2011/08/18 22:51:56, imasaki1 wrote: > > On 2011/08/18 19:56:58, Ami Fischman wrote: > > > Stopped reviewing at this point b/c I realized this is a parser for > > > test_expectations.txt, and surely we already have something for that. > > > > > > Indeed, > > > > > > http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/To... > > > is exactly that. > > > An example of using it from chromium python code (in case you want to avoid > > > upstreaming the analyzer to webkit) is > > > > > > http://src.chromium.org/viewvc/chrome/trunk/src/webkit/tools/layout_tests/web... > > > > This class is broken since test_expectations.TestExpectationsFile is deleted > by > > the change http://trac.webkit.org/changeset/92861. Also, there is class > location > > change. > > I think you meant http://trac.webkit.org/changeset/90934 > But that CL's description makes it seem like TestExpectationsFile was not needed > in the first place. > I wasn't suggesting you *use* expectations_line.py, only that it might serve as > a useful example for you for how to use test_expectations.py > > > > > Will that allow you to simply delete this file and its test? > > I have been trying on this for 3 hours or so. However, it is very difficult to > > use their package from chromium script. I think for now. I would like to keep > > as it is. Let me know what you think. > > What I think is that to do the job right here, you will end up with a copy of > the webkit module, anyway. > For example, in the webkit module I see AUDIO and NOW, which don't appear in > this file. I don't know if that's a bug here or there or in the > test_expectations.txt file's comments, but ISTM that having this copy that > nobody else knows about is nearly guaranteed to go stale very quickly. > > What difficulty did you run into using the webkit module from a chromium script? The problem I am having is that I cannot find an example, which uses parse functionality (with minimal efforts), As I said, http://src.chromium.org/viewvc/chrome/trunk/src/webkit/tools/layout_tests/web... does not work. Unit test also fails since it cannot find test_expectation module. http://src.chromium.org/viewvc/chrome/trunk/src/webkit/tools/layout_tests/web... It seems these sfuff are moved to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/models So, I was also trying to mimic http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/mode... However, it requires to set up port which I do not know.
> > So, I was also trying to mimic > http://trac.webkit.org/**browser/trunk/Tools/Scripts/** > webkitpy/layout_tests/models/**test_expectations_unittest.py#**L121<http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py#L121> > However, it requires to set up port which I do not know. > What happens if you try to do exactly what that test does?
On 2011/08/19 16:40:26, Ami Fischman wrote: > > > > So, I was also trying to mimic > > http://trac.webkit.org/**browser/trunk/Tools/Scripts/** > > > webkitpy/layout_tests/models/**test_expectations_unittest.py#**L121<http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py#L121> > > However, it requires to set up port which I do not know. > > > > What happens if you try to do exactly what that test does? I got error since I did not set port right. Correct port is necessary to initialize this object.
You made the same init call as the webkit test but it failed for you?
FTR: off-reviewlog Kenji had an exchange w/ dglazkov@ and abarth@ about depending on webkitpy code from chromium (and more generally from outside webkitpy) and was told that was a very bad idea b/c webkitpy doesn't expose an interface that can be expected to be stable. Thus to use webkitpy this code would have to live in webkitpy. Maybe someday that'll happen, but that day is not today. So resuming review below. http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... File media/tools/layout_tests/test_expectations.py (right): http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations.py:52: {'media/video-zoom.html': [{'LINUX':True, 'DEBUG':True ....}, Are the values always True/False? If so ISTM you'd want a list/vector, not a dict? http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations.py:58: """get all names relating to test expectation data. All comments should start with capital letter (please make a pass over the CL) http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations.py:116: return '' raise? http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations.py:119: def ParseLine(line, comments): |comments| should be |comment_prefix| or something like that, since it holds the comments from previous lines; this line may have yet more comments that should be appended to the group comment. http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations.py:136: if not line: Isn't this guarded against before calling this function? (even if it wasn't, you wouldn't want to be checking this inside the for-name loop) http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations.py:137: # Clear the comments if there is space (this is typical the case). s/typical/typically/ http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations.py:140: test_expectation_info[name] = True This is going to incorrectly count names appearing in comments as being non-comments. I think you need to strip off comments *first* in this function, and append them to comments. http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... File media/tools/layout_tests/test_expectations_unittest.py (right): http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_unittest.py:20: 'map result is different from what is expected.') FWIW the msg arg to assertEquals here and below seems unnecessary to me. http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_unittest.py:40: unittest.TextTestRunner(verbosity=2).run(test_suite) what is "2"?
Thanks! http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... File media/tools/layout_tests/test_expectations.py (right): http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations.py:52: {'media/video-zoom.html': [{'LINUX':True, 'DEBUG':True ....}, On 2011/08/20 04:16:50, Ami Fischman wrote: > Are the values always True/False? If so ISTM you'd want a list/vector, not a > dict? This map also contains 'Bugs' which holds a list of bugs. I prefer map since I can look them up directly. http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations.py:58: """get all names relating to test expectation data. On 2011/08/20 04:16:50, Ami Fischman wrote: > All comments should start with capital letter (please make a pass over the CL) Done. http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations.py:116: return '' On 2011/08/20 04:16:50, Ami Fischman wrote: > raise? Done. http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations.py:119: def ParseLine(line, comments): On 2011/08/20 04:16:50, Ami Fischman wrote: > |comments| should be |comment_prefix| or something like that, since it holds the > comments from previous lines; this line may have yet more comments that should > be appended to the group comment. Done. http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations.py:136: if not line: On 2011/08/20 04:16:50, Ami Fischman wrote: > Isn't this guarded against before calling this function? > (even if it wasn't, you wouldn't want to be checking this inside the for-name > loop) Deleted http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations.py:137: # Clear the comments if there is space (this is typical the case). On 2011/08/20 04:16:50, Ami Fischman wrote: > s/typical/typically/ Done. http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations.py:140: test_expectation_info[name] = True On 2011/08/20 04:16:50, Ami Fischman wrote: > This is going to incorrectly count names appearing in comments as being > non-comments. > I think you need to strip off comments *first* in this function, and append them Comments are dealt in __init__ function. So, the line here should not be comments. > to comments. http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... File media/tools/layout_tests/test_expectations_unittest.py (right): http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_unittest.py:20: 'map result is different from what is expected.') On 2011/08/20 04:16:50, Ami Fischman wrote: > FWIW the msg arg to assertEquals here and below seems unnecessary to me. Deleted. http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_unittest.py:40: unittest.TextTestRunner(verbosity=2).run(test_suite) On 2011/08/20 04:16:50, Ami Fischman wrote: > what is "2"? Deleted and made it simple.
Close. http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... File media/tools/layout_tests/test_expectations.py (right): http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations.py:116: return '' On 2011/08/20 08:25:36, imasaki1 wrote: > On 2011/08/20 04:16:50, Ami Fischman wrote: > > raise? > > Done. I meant raise a sensible exception. Doesn't this trigger a TypeError b/c NoneType is not a valid thing to raise? http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations.py:140: test_expectation_info[name] = True On 2011/08/20 08:25:36, imasaki1 wrote: > On 2011/08/20 04:16:50, Ami Fischman wrote: > > This is going to incorrectly count names appearing in comments as being > > non-comments. > > I think you need to strip off comments *first* in this function, and append > them > > Comments are dealt in __init__ function. So, the line here should not be > comments. I was referring to line-ending comments, not multi-line ones. They're apparently rare but exi$ grep // test_expectations.txt |grep -v '^//' BUGCR30536 SLOW LINUX WIN : http/tests/misc/acid3.html = FAIL // See "SVG Tests" section too BUGWK54322 SNOWLEOPARD : http/tests/misc/acid3.html = IMAGE // See "SVG tests" section too
Thanks for quick reply. http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... File media/tools/layout_tests/test_expectations.py (right): http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations.py:116: return '' On 2011/08/20 17:52:23, Ami Fischman wrote: > On 2011/08/20 08:25:36, imasaki1 wrote: > > On 2011/08/20 04:16:50, Ami Fischman wrote: > > > raise? > > > > Done. > > I meant raise a sensible exception. Doesn't this trigger a TypeError b/c > NoneType is not a valid thing to raise? Done. Also added unit test for this case. http://codereview.chromium.org/7671049/diff/4002/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations.py:140: test_expectation_info[name] = True On 2011/08/20 17:52:23, Ami Fischman wrote: > On 2011/08/20 08:25:36, imasaki1 wrote: > > On 2011/08/20 04:16:50, Ami Fischman wrote: > > > This is going to incorrectly count names appearing in comments as being > > > non-comments. > > > I think you need to strip off comments *first* in this function, and append > > them > > > > Comments are dealt in __init__ function. So, the line here should not be > > comments. > > I was referring to line-ending comments, not multi-line ones. > They're apparently rare but exi$ grep // test_expectations.txt |grep -v '^//' > BUGCR30536 SLOW LINUX WIN : http/tests/misc/acid3.html = FAIL // See "SVG Tests" > section too > BUGWK54322 SNOWLEOPARD : http/tests/misc/acid3.html = IMAGE // See "SVG tests" > section too Modified... Added unit test for this.
http://codereview.chromium.org/7671049/diff/15001/media/tools/layout_tests/te... File media/tools/layout_tests/test_expectations_unittest.py (right): http://codereview.chromium.org/7671049/diff/15001/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations_unittest.py:22: line = "BUGCR86714 MAC GPU : media/video-zoom.html = CRASH IMAGE // foo" I think you still missed my point. If the inline comment includes GPU for example, the expectation will include that as a name instead of just as a comment.
Thanks! http://codereview.chromium.org/7671049/diff/15001/media/tools/layout_tests/te... File media/tools/layout_tests/test_expectations_unittest.py (right): http://codereview.chromium.org/7671049/diff/15001/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations_unittest.py:22: line = "BUGCR86714 MAC GPU : media/video-zoom.html = CRASH IMAGE // foo" On 2011/08/20 18:21:00, fischman1 wrote: > I think you still missed my point. If the inline comment includes GPU for > example, the expectation will include that as a name instead of just as a > comment. Done.
LGTM
A bunch of minor comments ;-) http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... File media/tools/layout_tests/test_expectations.py (right): http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:6: """A Module for the Webkit layout test test expectation related class.""" If this is accurate, it might be a little more clear: "Analyzes test expectations for Webkit layout tests." http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:12: # TODO(imasaki) : support multiple test expectations files. nit: remove the space right before the ':' http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:43: the test expectation file. So, the map should keep all the occurrences nit: 'occurrences' --> 'occurrence' http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:50: information. In other words,this is the generated map after parsing both nit: put a space right after the comma http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:51: lines. nit: change the period to a colon . --> : http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:53: {'MAC':True, 'GPU':True ....}] nit: put a space after the colon in the above 2 lines within the map (4 instances to fix): ':True' --> ': True' http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:63: return DECISION_NAMES + PLATFORM_NAMES + CONFIG_NAMES + EXPECTATION_NAMES Rather than defining this as a static method, what about just defining it as a constant up above with the others (around line 31 above), like this: ALL_DATA_NAMES = (DECISION_NAMES + PLATFORM_NAMES + CONFIG_NAMES + EXPECTATION_NAMES) http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:71: such as 'SKIP' or 'Comments' or 'Bugs'. I don't quite understand this part of the comment: "...a list of the test expectation entries information map such as 'SKIP' or 'Comments' or 'Bugs'". If the below is still accurate, I think it's a little more clear: "All parsed information is stored into instance variable |all_test_expectation_info|, which is a dictionary mapping a string test case name to a list of dictionaries containing test expectation entry information." http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:74: url: A URL string for the test expectation file. Add a "Raises:" section to this docstring since an exception may be raised at line 79 below. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:83: for line in lines: We could combine lines 81 and 83 into this: for line in resp.read().split('\n'): http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:85: # Comments can be Multiple lines. nit: 'Multiple' --> 'multiple' http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:98: def ExtractTestName(line): Since this function extracts either a test or directory name, should the function name be changed to something like ExtractTestOrDirectoryName? http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:102: line: each line in the test expectation file. 'each line' --> 'a line' http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:105: a test name or directory name string. Returns '' if no matching. nit: 'matching' --> 'match' http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:106: """ Add a "Raises:" section to this docstring, since an exception can be explicitly raised at line 116 below. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:107: # First try to find test case where ending with .html. 'where' --> 'name' http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:109: # Next try to find directory 'directory' --> 'directory name' Also add a period at the end of the line. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:116: raise ValueError Should we attach an informative message along with this raised exception? http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:123: map and returns it. Also, store comments and bugs in the map. I think this might be slightly more clear: "This function checks for each entry from |GetAllDataNames()| in the current line and stores it in the test expectation info map if found. Comment and bug information is also stored in the map." http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:126: line: each line in the test expectation. For example, 'each line' --> 'a line' 'expectation' --> 'expectation file' http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:129: before the entry. This holds the comments from previous lines. comment_prefix: Comments from the test expectation file occurring just before the current line being parsed. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:132: a map containing the test expectation entries an and comments and bugs. 'a dictionary containing test expectation info, including comment and bug info.' http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:146: # Store bugs. nit: 'bugs' --> 'bug information' http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:148: if len(bugs) > 0: I think this is equivalent: if bugs: http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... File media/tools/layout_tests/test_expectations_unittest.py (right): http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations_unittest.py:15: comments = "Comments" Prefer single quotes over double quotes. Repeat in lines 23 and 31 below. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations_unittest.py:19: expected_map) msg='Some informative error message.' http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations_unittest.py:21: def testParseLineWithLineCommentsFoo(self): I think we can remove "Foo" from the function name. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations_unittest.py:27: expected_map) msg='Some informative error message.' http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations_unittest.py:35: expected_map) msg='Some informative error message.' http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations_unittest.py:42: 'Test case name is different') msg='Test case name is different.' http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations_unittest.py:48: 'Test case name is different') msg='Test case name is different.' http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations_unittest.py:56: fail Maybe we could use assertRaises() here: http://docs.python.org/library/unittest.html#unittest.TestCase.assertRaises
I hope I have addressed all issues. Thank you! http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... File media/tools/layout_tests/test_expectations.py (right): http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:6: """A Module for the Webkit layout test test expectation related class.""" On 2011/08/22 16:39:39, dennis_jeffrey wrote: > If this is accurate, it might be a little more clear: > > "Analyzes test expectations for Webkit layout tests." Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:12: # TODO(imasaki) : support multiple test expectations files. On 2011/08/22 16:39:39, dennis_jeffrey wrote: > nit: remove the space right before the ':' Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:43: the test expectation file. So, the map should keep all the occurrences On 2011/08/22 16:39:39, dennis_jeffrey wrote: > nit: 'occurrences' --> 'occurrence' Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:50: information. In other words,this is the generated map after parsing both On 2011/08/22 16:39:39, dennis_jeffrey wrote: > nit: put a space right after the comma Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:51: lines. On 2011/08/22 16:39:39, dennis_jeffrey wrote: > nit: change the period to a colon > > . --> : Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:53: {'MAC':True, 'GPU':True ....}] On 2011/08/22 16:39:39, dennis_jeffrey wrote: > nit: put a space after the colon in the above 2 lines within the map (4 > instances to fix): > > ':True' --> ': True' Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:63: return DECISION_NAMES + PLATFORM_NAMES + CONFIG_NAMES + EXPECTATION_NAMES On 2011/08/22 16:39:39, dennis_jeffrey wrote: > Rather than defining this as a static method, what about just defining it as a > constant up above with the others (around line 31 above), like this: > > ALL_DATA_NAMES = (DECISION_NAMES + PLATFORM_NAMES + CONFIG_NAMES + > EXPECTATION_NAMES) Done. Also rename this variable http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:71: such as 'SKIP' or 'Comments' or 'Bugs'. On 2011/08/22 16:39:39, dennis_jeffrey wrote: > I don't quite understand this part of the comment: "...a list of the test > expectation entries information map such as 'SKIP' or 'Comments' or 'Bugs'". If > the below is still accurate, I think it's a little more clear: > > "All parsed information is stored into instance variable > |all_test_expectation_info|, which is a dictionary mapping a string test case > name to a list of dictionaries containing test expectation entry information." Done. Also added example http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:74: url: A URL string for the test expectation file. On 2011/08/22 16:39:39, dennis_jeffrey wrote: > Add a "Raises:" section to this docstring since an exception may be raised at > line 79 below. Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:83: for line in lines: On 2011/08/22 16:39:39, dennis_jeffrey wrote: > We could combine lines 81 and 83 into this: > > for line in resp.read().split('\n'): Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:85: # Comments can be Multiple lines. On 2011/08/22 16:39:39, dennis_jeffrey wrote: > nit: 'Multiple' --> 'multiple' Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:98: def ExtractTestName(line): On 2011/08/22 16:39:39, dennis_jeffrey wrote: > Since this function extracts either a test or directory name, should the > function name be changed to something like ExtractTestOrDirectoryName? Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:102: line: each line in the test expectation file. On 2011/08/22 16:39:39, dennis_jeffrey wrote: > 'each line' --> 'a line' Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:105: a test name or directory name string. Returns '' if no matching. On 2011/08/22 16:39:39, dennis_jeffrey wrote: > nit: 'matching' --> 'match' Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:106: """ On 2011/08/22 16:39:39, dennis_jeffrey wrote: > Add a "Raises:" section to this docstring, since an exception can be explicitly > raised at line 116 below. Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:107: # First try to find test case where ending with .html. On 2011/08/22 16:39:39, dennis_jeffrey wrote: > 'where' --> 'name' Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:109: # Next try to find directory On 2011/08/22 16:39:39, dennis_jeffrey wrote: > 'directory' --> 'directory name' > > Also add a period at the end of the line. Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:116: raise ValueError On 2011/08/22 16:39:39, dennis_jeffrey wrote: > Should we attach an informative message along with this raised exception? Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:123: map and returns it. Also, store comments and bugs in the map. On 2011/08/22 16:39:39, dennis_jeffrey wrote: > I think this might be slightly more clear: > > "This function checks for each entry from |GetAllDataNames()| in the current > line and stores it in the test expectation info map if found. Comment and bug > information is also stored in the map." Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:126: line: each line in the test expectation. For example, On 2011/08/22 16:39:39, dennis_jeffrey wrote: > 'each line' --> 'a line' > > 'expectation' --> 'expectation file' Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:129: before the entry. This holds the comments from previous lines. On 2011/08/22 16:39:39, dennis_jeffrey wrote: > comment_prefix: Comments from the test expectation file occurring just before > the current line being parsed. Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:132: a map containing the test expectation entries an and comments and bugs. On 2011/08/22 16:39:39, dennis_jeffrey wrote: > 'a dictionary containing test expectation info, including comment and bug info.' Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:146: # Store bugs. On 2011/08/22 16:39:39, dennis_jeffrey wrote: > nit: 'bugs' --> 'bug information' Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:148: if len(bugs) > 0: On 2011/08/22 16:39:39, dennis_jeffrey wrote: > I think this is equivalent: > > if bugs: Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... File media/tools/layout_tests/test_expectations_unittest.py (right): http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations_unittest.py:15: comments = "Comments" On 2011/08/22 16:39:39, dennis_jeffrey wrote: > Prefer single quotes over double quotes. Repeat in lines 23 and 31 below. Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations_unittest.py:19: expected_map) On 2011/08/22 16:39:39, dennis_jeffrey wrote: > msg='Some informative error message.' Discussed with Dennis offline. This can be kept as it is. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations_unittest.py:21: def testParseLineWithLineCommentsFoo(self): On 2011/08/22 16:39:39, dennis_jeffrey wrote: > I think we can remove "Foo" from the function name. Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations_unittest.py:27: expected_map) On 2011/08/22 16:39:39, dennis_jeffrey wrote: > msg='Some informative error message.' Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations_unittest.py:35: expected_map) On 2011/08/22 16:39:39, dennis_jeffrey wrote: > msg='Some informative error message.' Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations_unittest.py:42: 'Test case name is different') On 2011/08/22 16:39:39, dennis_jeffrey wrote: > msg='Test case name is different.' Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations_unittest.py:48: 'Test case name is different') On 2011/08/22 16:39:39, dennis_jeffrey wrote: > msg='Test case name is different.' Done. http://codereview.chromium.org/7671049/diff/12003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations_unittest.py:56: fail On 2011/08/22 16:39:39, dennis_jeffrey wrote: > Maybe we could use assertRaises() here: > > http://docs.python.org/library/unittest.html#unittest.TestCase.assertRaises Done.
LGTM Just 3 minor comments to address before submitting. Thank you! http://codereview.chromium.org/7671049/diff/18003/media/tools/layout_tests/te... File media/tools/layout_tests/test_expectations.py (right): http://codereview.chromium.org/7671049/diff/18003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:6: """A Module to analyze test expectations for Webkit layout tests""" nit: add period at end of the sentence right after 'tests' http://codereview.chromium.org/7671049/diff/18003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:58: BUGCR86714 LINUX DEBUG : media/video-zoom.html = IMAGE This information is already given in lines 48-49 above. http://codereview.chromium.org/7671049/diff/18003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:105: dictionary: Such examples are: 'dictionary' --> 'directory'
Thanks. Committed. http://codereview.chromium.org/7671049/diff/18003/media/tools/layout_tests/te... File media/tools/layout_tests/test_expectations.py (right): http://codereview.chromium.org/7671049/diff/18003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:6: """A Module to analyze test expectations for Webkit layout tests""" On 2011/08/22 18:59:24, dennis_jeffrey wrote: > nit: add period at end of the sentence right after 'tests' Done. http://codereview.chromium.org/7671049/diff/18003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:58: BUGCR86714 LINUX DEBUG : media/video-zoom.html = IMAGE On 2011/08/22 18:59:24, dennis_jeffrey wrote: > This information is already given in lines 48-49 above. Removed from here.
Thanks. Committed. http://codereview.chromium.org/7671049/diff/18003/media/tools/layout_tests/te... File media/tools/layout_tests/test_expectations.py (right): http://codereview.chromium.org/7671049/diff/18003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:6: """A Module to analyze test expectations for Webkit layout tests""" On 2011/08/22 18:59:24, dennis_jeffrey wrote: > nit: add period at end of the sentence right after 'tests' Done. http://codereview.chromium.org/7671049/diff/18003/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations.py:58: BUGCR86714 LINUX DEBUG : media/video-zoom.html = IMAGE On 2011/08/22 18:59:24, dennis_jeffrey wrote: > This information is already given in lines 48-49 above. Removed from here. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
