Bug 57298

Summary: Add tests to PrettyPatch
Product: WebKit Reporter: Caio Marcelo de Oliveira Filho <cmarcelo>
Component: Tools / TestsAssignee: 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 Flags
Patch
aroben: review+
patch v2, add descriptions for all cases tested none

Description Caio Marcelo de Oliveira Filho 2011-03-28 22:22:44 PDT
Add tests to PrettyPatch
Comment 1 Caio Marcelo de Oliveira Filho 2011-03-28 22:28:22 PDT
Created attachment 87260 [details]
Patch
Comment 2 Caio Marcelo de Oliveira Filho 2011-03-28 22:36:40 PDT
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 3 Adam Roben (:aroben) 2011-04-19 07:10:20 PDT
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.
Comment 4 Caio Marcelo de Oliveira Filho 2011-04-19 08:45:45 PDT
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. :-)
Comment 5 Caio Marcelo de Oliveira Filho 2011-04-28 18:25:08 PDT
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.
Comment 6 Caio Marcelo de Oliveira Filho 2011-05-03 14:09:40 PDT
Committed in

http://trac.webkit.org/changeset/85657
Comment 7 Caio Marcelo de Oliveira Filho 2011-05-03 14:10:01 PDT
Comment on attachment 91612 [details]
patch v2, add descriptions for all cases tested

Clearing flags.