Bug 12140 – Change remaining references in tests from bugzilla.opendarwin.org to bugs.webkit.org

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[David Kilzer (:ddkilzer)]

Just realized that many layout tests contain URLs to bugzilla.opendarwin.org.  It would be nice to change them to bugs.webkit.org eventually.

[David Kilzer (:ddkilzer)]

Some fixes applied in r20304 by bdash.

[Mark Rowe (bdash)]

I didn't touch any layout tests in my change -- regenerating a whole bunch of results for a change like this didn't seem like a great idea.

[David Kilzer (:ddkilzer)]

Genericized the title.  :)

[Shinichiro Hamaji]

Created attachment 40231 [details]
Patch v1

[Shinichiro Hamaji]

How about modifying all dumpAsText tests? Changing them may be harmless because we don't need to re-generate test results for all architectures we support.

The following two-liners created this patch:

% grep bugzilla.opendarwin.org -r --include='*.html' LayoutTests/*/* | perl -ne 's/:.*//; $e=$_; $e=~s/\.html/-expected.txt/; chomp($e); print if -e $e' > l
% perl -i -p -e 's@http://bugzilla.opendarwin.org/@http://bugs.webkit.org/@g' `cat l`

Note that not all test results didn't require change of test results because some URLs were in href attribute, which doesn't affect test results. So, I think we can modify such links even for non-dumpAsText tests, too. If other people like this idea, I'll be happy to work on it.

[David Kilzer (:ddkilzer)]

(In reply to comment #4)
> Created an attachment (id=40231) [details]
> Patch v1

The commands in Comment #5 don't test for the existence of pixel test results.  I know that we're not running a buildbot for pixel tests now, but we should only change the dumpAsText tests.  How do you know the tests that were changed don't have any pixel test results?

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> How about modifying all dumpAsText tests? Changing them may be harmless because
> we don't need to re-generate test results for all architectures we support.

That sounds fine.

> The following two-liners created this patch:
> 
> % grep bugzilla.opendarwin.org -r --include='*.html' LayoutTests/*/* | perl -ne
> 's/:.*//; $e=$_; $e=~s/\.html/-expected.txt/; chomp($e); print if -e $e' > l
> % perl -i -p -e 's@http://bugzilla.opendarwin.org/@http://bugs.webkit.org/@g'
> `cat l`
> 
> Note that not all test results didn't require change of test results because
> some URLs were in href attribute, which doesn't affect test results. So, I
> think we can modify such links even for non-dumpAsText tests, too. If other
> people like this idea, I'll be happy to work on it.

Yes, if it doesn't affect the pixel test results or render tree dumps, that sounds fine.

You should also consider changing 'http://bugzilla.opendarwin.org/' to 'https://bugs.webkit.org/' while you're at it.  (It's not necessary to change 'http://bugs.webkit.org/' to 'https://bugs.webkit.org/', though, since those links still work fine.)

[Shinichiro Hamaji]

> The commands in Comment #5 don't test for the existence of pixel test results. 
> I know that we're not running a buildbot for pixel tests now, but we should
> only change the dumpAsText tests.  How do you know the tests that were changed
> don't have any pixel test results?

Oh... I thought checking existence of -expected.txt is sufficient. There can be tests which have both -expected.txt and pixel test results? I'll update my patch tomorrow.

Also, I'll do s/http/https/g for my patch as you pointed.

After the dumpAsText patch is committed, I'll send another patch for href attributes.

Thank you for the comments!

[Shinichiro Hamaji]

Created attachment 40290 [details]
Patch v2

[Shinichiro Hamaji]

Created attachment 40291 [details]
a script to find dumpAsText tests

I updated my first line of the previous script so it checks if there are -expected.png . It seems that there were no pixel tests which have -expected.txt and opendarwin URL. I also confirmed pixel tests for these HTMLs succeeded. So, the patch v2 isn't different from the previous patch except s/http/https/g .

[David Kilzer (:ddkilzer)]

Comment on attachment 40290 [details]
Patch v2

r=me  Thanks!

[WebKit Commit Bot]

Comment on attachment 40290 [details]
Patch v2

Rejecting patch 40290 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
tml
        trunk/LayoutTests/fast/encoding/charset-koi8-u.html
        trunk/LayoutTests/fast/frames/frame-name-reset.html
        trunk/LayoutTests/fast/frames/iframe-name-and-id.html
        trunk/LayoutTests/fast/loader/url-strip-cr-lf-tab.html
        trunk/LayoutTests/fast/overflow/generated-content-crash.html

    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
 at /usr/local/libexec/git-core//git-svn line 469

[Shinichiro Hamaji]

Created attachment 40310 [details]
Patch v1

[Shinichiro Hamaji]

s/v1/v3/

OK... I've just eliminated all tabs in these HTMLs. I believe this is better than adding allow-tabs properties for them. If I'm wrong, please let me know and I'll svn propset for them.

[David Kilzer (:ddkilzer)]

Comment on attachment 40310 [details]
Patch v1

Yay, fewer tabs!  r=me

[WebKit Commit Bot]

Comment on attachment 40310 [details]
Patch v1

Clearing flags on attachment: 40310

Committed r48891: <http://trac.webkit.org/changeset/48891>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> After the dumpAsText patch is committed, I'll send another patch for href
> attributes.

Reopening.

[Shinichiro Hamaji]

Created attachment 40347 [details]
Patch for hrefs v1

[Shinichiro Hamaji]

(In reply to comment #19)
> Created an attachment (id=40347) [details]
> Patch for hrefs v1

This patch modifies opendarwin links in href attributes. Note that I also eliminated tabs in these files. The following two files are using tabs intentionally. I added allow-tabs properties for them.

LayoutTests/fast/css-generated-content/hover-style-change.html
LayoutTests/fast/dom/Element/class-attribute-whitespace.html

Here is the scripts I used.

% grep -l 'href="http://bugzilla.opendarwin.org' -r --include='*.html' LayoutTests/*/* > l
% perl -i -p -e 's@href="http://bugzilla.opendarwin.org/@href="https://bugs.webkit.org/@g' `cat l`

# This file contained whitespaces in its file name.
% perl -i -p -e 's@href="http://bugzilla.opendarwin.org/@href="https://bugs.webkit.org/@g' LayoutTests/fast/encoding/resources/%25%u0435\ 0\ %xx%%%ulike.html

# Run pixel tests excluding resources/*.html
% ./WebKitTools/Scripts/run-webkit-tests --release -p `grep -v 'resources/' l`

# Check the list of resources/*.html and invoke test manually for them.
% grep resources l
LayoutTests/fast/events/resources/drag-outside-window-frame.html
LayoutTests/fast/history/resources/clicked-link-is-visited-2.html
% ./WebKitTools/Scripts/run-webkit-tests --release -p LayoutTests/fast/events/drag-outside-window.html
% ./WebKitTools/Scripts/run-webkit-tests --release -p LayoutTests/fast/history/clicked-link-is-visited.html
% ./WebKitTools/Scripts/run-webkit-tests --release -p LayoutTests/fast/encoding/percent-escaping.html

[Eric Seidel (no email)]

Comment on attachment 40347 [details]
Patch for hrefs v1

?       svn-commit.2.tmp
?       svn-commit.3.tmp
?       svn-commit.4.tmp
?       make-script-test-wrappers
?       svn-commit.5.tmp
?       patch
?       tests
?       log2
?       gitp
?       log
?       d
?       svn-commit.tmp
?       m
?       p
?       old_props
?       diff.rb

Shouldn't be in the diff, but svn-apply will ignore it.

This looks fine to me.

[Shinichiro Hamaji]

Committed r48921: <http://trac.webkit.org/changeset/48921>

[Shinichiro Hamaji]

> Shouldn't be in the diff, but svn-apply will ignore it.
> 
> This looks fine to me.

I've committed manually to make sure the unnecessary lines don't cause any errors.

This seems SCM.run_command in scm.py is redirecting stderr to stdout and the unnecessary lines were added.

  process = subprocess.Popen(args, stdin=stdin, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, cwd=cwd)

Maybe we should file a bug for this?

[Eric Seidel (no email)]

Oh true.  That's a recent regression.  Please do file a bug about that.

[Shinichiro Hamaji]

Created attachment 40350 [details]
Patch for JS v1

[Shinichiro Hamaji]

Reopening this bug for fix of JS. They are used by dumpAsText tests so it should be safe to modify the test expectations.

[David Kilzer (:ddkilzer)]

Comment on attachment 40350 [details]
Patch for JS v1

r=me

[WebKit Commit Bot]

Comment on attachment 40350 [details]
Patch for JS v1

Clearing flags on attachment: 40350

Committed r48925: <http://trac.webkit.org/changeset/48925>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.