Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2841483007:
MHTMLTest::TestMHTMLEncoding checks generated MTHML is well formed. (Closed)
DescriptionMHTMLTest::TestMHTMLEncoding checks generated MTHML is well formed.
After generating MHTML data in memory checks that it is well formed by
parsing it using MHTMLParser.
BUG=672313
Review-Url: https://codereview.chromium.org/2841483007
Cr-Commit-Position: refs/heads/master@{#467125}
Committed: https://chromium.googlesource.com/chromium/src/+/8201c9cbc87f806a3fce3df667b36221c600caf0
Patch Set 1 #
Total comments: 2
Patch Set 2 : Move validation into GenerateMHTMLData. #
Total comments: 2
Patch Set 3 : Using ADD_FAILURE and HasFailure. #
Total comments: 1
Messages
Total messages: 18 (9 generated)
The CQ bit was checked by carlosk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
carlosk@chromium.org changed reviewers: + dcheng@chromium.org, lukasza@chromium.org
dcheng@, lukasza@: PTAL.
Thanks for adding the extra validation. This looks good and seems like a very desirable change, but I am curious if with some small tweaks we can have this extra validation on more tests - see the question below. https://codereview.chromium.org/2841483007/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/tests/MHTMLTest.cpp (right): https://codereview.chromium.org/2841483007/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/MHTMLTest.cpp:213: ASSERT_FALSE(parser.ParseArchive().IsEmpty()) << "Invalid MHTML"; Would it be possible to increase the coverage of the new check by moving the check to MHTMLTest::GenerateMHTMLData? If some callers of MHTMLTest::GenerateMHTMLData expect creation of malformed MHTML, then maybe we can add a parameter to MHTMLTest::GenerateMHTMLData (or a field to MHTMLTest?) that tells the code whether the asserts above are appropriate.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks. https://codereview.chromium.org/2841483007/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/tests/MHTMLTest.cpp (right): https://codereview.chromium.org/2841483007/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/MHTMLTest.cpp:213: ASSERT_FALSE(parser.ParseArchive().IsEmpty()) << "Invalid MHTML"; On 2017/04/25 19:01:24, Łukasz A. wrote: > Would it be possible to increase the coverage of the new check by moving the > check to MHTMLTest::GenerateMHTMLData? If some callers of > MHTMLTest::GenerateMHTMLData expect creation of malformed MHTML, then maybe we > can add a parameter to MHTMLTest::GenerateMHTMLData (or a field to MHTMLTest?) > that tells the code whether the asserts above are appropriate. Done.
LGTM, but I have one more suggestion below (or rather a clarification of what I meant in my previous response :-). https://codereview.chromium.org/2841483007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/MHTMLTest.cpp (right): https://codereview.chromium.org/2841483007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/MHTMLTest.cpp:170: return RawData::Create(); Would it be okay if the above said: if (parser.ParseArchive().IsEmpty()) { ADD_FAILURE() << "Invalid MHTML"; return RawData::Create(); } This way all callers of MHTMLArchive::GenerateMHTMLFooterForTesting will go through verification that the generated mhtml is well-formed.
Thanks. Let me know if the way I'm checking for failures is not OK. https://codereview.chromium.org/2841483007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/MHTMLTest.cpp (right): https://codereview.chromium.org/2841483007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/MHTMLTest.cpp:170: return RawData::Create(); On 2017/04/25 20:00:45, Łukasz A. wrote: > Would it be okay if the above said: > > if (parser.ParseArchive().IsEmpty()) { > ADD_FAILURE() << "Invalid MHTML"; > return RawData::Create(); > } > > This way all callers of MHTMLArchive::GenerateMHTMLFooterForTesting will go > through verification that the generated mhtml is well-formed. Done.
The CQ bit was checked by carlosk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lukasza@chromium.org Link to the patchset: https://codereview.chromium.org/2841483007/#ps40001 (title: "Using ADD_FAILURE and HasFailure.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/25 20:18:15, carlosk wrote: > Thanks. Let me know if the way I'm checking for failures is not OK. > Thanks - looks good :-)
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493151499115290,
"parent_rev": "feafc2de2c3e515247b5f45ab9ac9a4139b6cd46", "commit_rev":
"8201c9cbc87f806a3fce3df667b36221c600caf0"}
Message was sent while issue was closed.
Description was changed from ========== MHTMLTest::TestMHTMLEncoding checks generated MTHML is well formed. After generating MHTML data in memory checks that it is well formed by parsing it using MHTMLParser. BUG=672313 ========== to ========== MHTMLTest::TestMHTMLEncoding checks generated MTHML is well formed. After generating MHTML data in memory checks that it is well formed by parsing it using MHTMLParser. BUG=672313 Review-Url: https://codereview.chromium.org/2841483007 Cr-Commit-Position: refs/heads/master@{#467125} Committed: https://chromium.googlesource.com/chromium/src/+/8201c9cbc87f806a3fce3df667b3... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/8201c9cbc87f806a3fce3df667b3...
Message was sent while issue was closed.
https://codereview.chromium.org/2841483007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/MHTMLTest.cpp (right): https://codereview.chromium.org/2841483007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/MHTMLTest.cpp:219: ASSERT_FALSE(HasFailure()); Per https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid..., we should either just change Serialize() to use an out param (and use FAIL()) or be content with a non-fatal assertion. It's a pretty unusual pattern to require callers to check HasFailure(), as that means that this has to be duplicated everywhere. |
