Summary: | Add tests to PrettyPatch | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Caio Marcelo de Oliveira Filho <cmarcelo> | ||||||
Component: | Tools / Tests | Assignee: | Caio Marcelo de Oliveira Filho <cmarcelo> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | aroben, cmarcelo, dbates, eric | ||||||
Priority: | P4 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Other | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Caio Marcelo de Oliveira Filho
2011-03-28 22:22:44 PDT
Created attachment 87260 [details]
Patch
Hello, as suggested in bug 21222, here is a first shot on making tests for PrettyPatch. I'm adding you (Eric and Adam) since the suggestion came from you. I would really appreciate some Ruby guidance, since I'm not familiar with the idioms. I also focused on the parsing, and not the HTML generation. I think is ok for now. Having tests is convenient, since I plan at some point to improve the parsing code and make it use the information of the ranges earilir instead of trying to do the parsing in separate phases: first dividing the patch in file sections, then finding the chunks inside. This kind of improvement will be helpful for bug 54928, for example. Comment on attachment 87260 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87260&action=review > Websites/bugs.webkit.org/PrettyPatch/PrettyPatch_test.rb:24 > + 20528 => [nil, 1, 4, 3, 7], > + 21120 => [nil, 2, 4, 2, 6], > + 21151 => [nil, 4, 9, 1, 16], > + 21152 => [nil, 2, 17, 7, 23], > + 21388 => [nil, 3, 4, 2, 6], Why no titles for these patches? > Websites/bugs.webkit.org/PrettyPatch/PrettyPatch_test.rb:45 > + def get_patch_uri(id) > + "https://bugs.webkit.org/attachment.cgi?id=" + id.to_s > + end > + > + def get_patch(id) > + result = nil > + patch_uri = get_patch_uri(id) > + begin > + result = open(patch_uri) { |f| result = f.read } > + rescue => exception > + assert(false, "Fail to get patch " + patch_uri) > + end > + result > + end It seems a little unfortunate for these tests to have to hit the network. Adam, thanks for the review. (In reply to comment #3) > (From update of attachment 87260 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87260&action=review > > > Websites/bugs.webkit.org/PrettyPatch/PrettyPatch_test.rb:24 > > + 20528 => [nil, 1, 4, 3, 7], > > + 21120 => [nil, 2, 4, 2, 6], > > + 21151 => [nil, 4, 9, 1, 16], > > + 21152 => [nil, 2, 17, 7, 23], > > + 21388 => [nil, 3, 4, 2, 6], > > Why no titles for these patches? I couldn't find a simple way to describe them. But I'll try harder. Some of them came from to previous bug reports of PrettyPatch, maybe in the worst case I can point to the bug report. > It seems a little unfortunate for these tests to have to hit the network. Hmm. For some reason it felt awkward to add the patches themselves to WebKit repo and was unsure about what Subversion would do the files with different line endings. On the other hand I wanted to test the "real cases". So, I think we can live with the test depending on network for now, but I can add them to the repository if you prefer. :-) Created attachment 91612 [details]
patch v2, add descriptions for all cases tested
Removed patches that had repeated "reason" to be tested.
As discussed in IRC, we can live with test that depends on network for now.
Committed in http://trac.webkit.org/changeset/85657 Comment on attachment 91612 [details]
patch v2, add descriptions for all cases tested
Clearing flags.
|