Bug 57298 – Add tests to PrettyPatch

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Caio Marcelo de Oliveira Filho]

Add tests to PrettyPatch

[Caio Marcelo de Oliveira Filho]

Created attachment 87260 [details]
Patch

[Caio Marcelo de Oliveira Filho]

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.

[Adam Roben (:aroben)]

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.

[Caio Marcelo de Oliveira Filho]

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. :-)

[Caio Marcelo de Oliveira Filho]

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.

[Caio Marcelo de Oliveira Filho]

Committed in

http://trac.webkit.org/changeset/85657

[Caio Marcelo de Oliveira Filho]

Comment on attachment 91612 [details]
patch v2, add descriptions for all cases tested

Clearing flags.