Bug 59240 – rebaseline-chromium-webkit-tests: clean up output

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

rebaseline-chromium-webkit-tests: clean up output

[Dirk Pranke]

Created attachment 90771 [details]
Patch

[Dirk Pranke]

not quite ready for review ... the tests I was using to validate and fine-tune this appear to be have been rebaselined out from under me :), and I still need to run the unit tests to make sure nothing broke.

I will post a sample of the new output ASAP.

[Dirk Pranke]

okay, unit tests pass, which makes sense since I didn't really change any functionality.

[Dirk Pranke]

Comment on attachment 90771 [details]
Patch

more tweaking to come, kill review?.

[Dirk Pranke]

Created attachment 90820 [details]
Patch

[Dirk Pranke]

Created attachment 90821 [details]
sample new output

[Dirk Pranke]

Right-o. I'm pretty happy with the output now (which I have attached). Comments? Review?

[Adam Barth]

Comment on attachment 90820 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90820&action=review

> Tools/Scripts/webkitpy/common/checkout/scm.py:716
> +        return return_code != 128

128 seems like a magical number here.  Can we name the constant?

[Dirk Pranke]

(In reply to comment #8)
> (From update of attachment 90820 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90820&action=review
> 
> > Tools/Scripts/webkitpy/common/checkout/scm.py:716
> > +        return return_code != 128
> 
> 128 seems like a magical number here.  Can we name the constant?

Sure. I have no explanation for it other than that it is what Git seems to return, but I'll add something. Git does not appear to document error codes at all.

[Dirk Pranke]

Created attachment 90834 [details]
update w/ more unit testing

[Dirk Pranke]

Created attachment 90835 [details]
delta between the last two patches

[Dirk Pranke]

Okay, it turns out the SCM unittests didn't actually pass, and (at least on the mac) attempting to get Git to deal with absolute and relative paths is a nightmare, so I've revised the patch to work around git's brokenness.

I also tweaked the way scm_unittest restores the current working directory to work if you were running the file from the command line (in which case __file__ isn't an absolute path)

[Adam Barth]

Comment on attachment 90835 [details]
delta between the last two patches

View in context: https://bugs.webkit.org/attachment.cgi?id=90835&action=review

> Tools/Scripts/webkitpy/common/checkout/scm_unittest.py:465
> +    def _shared_test_exists(self, scm, commit_fn):

commit_fn should be named something without abbreviations.

[Adam Barth]

Comment on attachment 90834 [details]
update w/ more unit testing

modulo naming nit

[Dirk Pranke]

Committed r84730: <http://trac.webkit.org/changeset/84730>

[Eric Seidel (no email)]

Comment on attachment 90834 [details]
update w/ more unit testing

View in context: https://bugs.webkit.org/attachment.cgi?id=90834&action=review

I'm not sure this patch is right.

> Tools/Scripts/webkitpy/common/checkout/scm.py:727
> +        return_code = self.run(["git", "show", "HEAD:%s" % path], return_exit_code=True, decode_output=False)

The way to do this is by passing cwd, no?

> Tools/Scripts/webkitpy/common/checkout/scm_unittest.py:183
> +        os.chdir(detect_scm_system(path).checkout_root)

Eeek. Why?  We've been trying to remove these.

> Tools/Scripts/webkitpy/common/checkout/scm_unittest.py:466
> +        os.chdir(scm.checkout_root)

Bleh.

[Dirk Pranke]

Comment on attachment 90834 [details]
update w/ more unit testing

View in context: https://bugs.webkit.org/attachment.cgi?id=90834&action=review

>> Tools/Scripts/webkitpy/common/checkout/scm_unittest.py:466
>> +        os.chdir(scm.checkout_root)
> 
> Bleh.

since scm requires all the paths to be relative to checkout_root, this is the easiest thing to do, but I'm open to alternative suggestions.

It seems like we should change SCM to not require paths to be relative to checkout_root, and just deal with absolute paths and paths relative to the cwd like every other filesystem interface under the sun :).

I'll take another look at this tomorrow and see if I can think of a better way of doing this patch without rewriting all of scm and scm_unittest.

[Dirk Pranke]

(In reply to comment #16)
> (From update of attachment 90834 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90834&action=review
> 
> I'm not sure this patch is right.
> 
> > Tools/Scripts/webkitpy/common/checkout/scm.py:727
> > +        return_code = self.run(["git", "show", "HEAD:%s" % path], return_exit_code=True, decode_output=False)
> 
> The way to do this is by passing cwd, no?
> 

Possibly. Does subprocess cd back to the previous dir after running the command?

> > Tools/Scripts/webkitpy/common/checkout/scm_unittest.py:183
> > +        os.chdir(detect_scm_system(path).checkout_root)
> 
> Eeek. Why?  We've been trying to remove these.
> 

If we didn't do this, the test wouldn't be self contained. Am I missing something?

[Eric Seidel (no email)]

Comment on attachment 90834 [details]
update w/ more unit testing

I can't remember, but I think we decided to accept and return all paths as relative to checkout_root.  But we do provide conversion functions, don't we?  Certainly os.relpath makes it easy to convert to checkout_root relative paths.

[Eric Seidel (no email)]

I'm of the opinion that every use of chdir in our codebase is wrong. :(  We had several from the dawn of time but have been trying to kill them.  I could be wrong of course.  But I think we should avoid adding more.

[Eric Seidel (no email)]

(In reply to comment #18)
> (In reply to comment #16)
> > (From update of attachment 90834 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=90834&action=review
> > 
> > I'm not sure this patch is right.
> > 
> > > Tools/Scripts/webkitpy/common/checkout/scm.py:727
> > > +        return_code = self.run(["git", "show", "HEAD:%s" % path], return_exit_code=True, decode_output=False)
> > 
> > The way to do this is by passing cwd, no?
> > 
> 
> Possibly. Does subprocess cd back to the previous dir after running the command?

It never leaves the directory. :)  It just sets PWD in the environment of the subprocess.  It's how we run most (if not all?) commands in Git.  I think Maciej added some different logic for SVN to support runing webkit-patch on subdirectories.  (I don't really support the svn codepath anymore given that I haven't used svn in years.)

[Dirk Pranke]

(In reply to comment #19)
> (From update of attachment 90834 [details])
> I can't remember, but I think we decided to accept and return all paths as relative to checkout_root.  But we do provide conversion functions, don't we?  Certainly os.relpath makes it easy to convert to checkout_root relative paths.

You'd think that, wouldn't you? Except Git doesn't seem to like that so much, at least not on the Mac, where we write stuff under tempdir which resolves to /var/tmp sometimes and /private/var/tmp if you call abspath() (or something like that), which is why I had to fix the earlier version of the patch.

I didn't look for conversion functions. I will do so.

(In reply to comment #20)
> I'm of the opinion that every use of chdir in our codebase is wrong. :(  We had several from the dawn of time but have been trying to kill them.  I could be wrong of course.  But I think we should avoid adding more.

I wouldn't normally argue that chdir() is wrong, but I'm not a big fan of making the tests hard to follow, either. I'll see if I can come up with a happy medium.

(In reply to comment #21)
> > Possibly. Does subprocess cd back to the previous dir after running the command?
> 
> It never leaves the directory. :)  It just sets PWD in the environment of the subprocess. 

Ah. even better.

[Eric Seidel (no email)]

You may be finding that Git/SVN classes in scm.py are inconsistent in how they treat paths.  scm.py is the second oldest file in webkitpy from when Adam and I were even more incompetent python programmers. :)

Again, my memory is foggy here, but I think that Git at least tries to deal with paths relative to the checkout_root.  I know there are a bunch of FIXMEs in the code about passing cwd=self.checkout_root more. :)

SVN intentionally avoids that (which I don't entirely agree with) as a way to make SVN commands not take 3.5 years to run.  (Since running anything from the svn root goes out and has tea and cookies with some set of servers before returning an answer. :)

You were right in your earlier comments that the scm_unittests are commonly broken.  We don't run them by default in test-webkitpy because they're so slow.  The long-term plan for that was to move some of them away from using real checkouts, and to make checkout creation faster (creating a git repo from svn seems to take forever).

Anyway, that's what I know.  Thanks again for the patches.

[Dirk Pranke]

(In reply to comment #23)
> You may be finding that Git/SVN classes in scm.py are inconsistent in how they treat paths.  scm.py is the second oldest file in webkitpy from when Adam and I were even more incompetent python programmers. :)
> 
> Again, my memory is foggy here, but I think that Git at least tries to deal with paths relative to the checkout_root.  I know there are a bunch of FIXMEs in the code about passing cwd=self.checkout_root more. :)
> 

Mostly the problem seems to be that Git itself doesn't deal with absolute paths well, and it doesn't deal with relative paths under the tree either (e.g. git show HEAD:test-webkitpy when run in the Tools/Scripts directory). I consider both of these things rather badly broken and would love it if the SCM abstraction hid them.

I don't think SVN has this problem.

> You were right in your earlier comments that the scm_unittests are commonly broken. 

Sorry, no, I meant that the tests I wrote *in this patch* were broken. Frankly, I was in too much of a hurry to run the whole test suite myself, so I have no idea if the other tests work or not :).

I'll check this as well tomorrow. My comments were not meant in any way as a criticism of your code, but rather the underlying stupidity of Git, Python, and the Mac filesystem (I mean, honestly, couldn't the porters have just returned /private/var/tmp? And what's with the ridiculous generated directory names?).

[Eric Seidel (no email)]

(In reply to comment #24)
> Mostly the problem seems to be that Git itself doesn't deal with absolute paths well, and it doesn't deal with relative paths under the tree either (e.g. git show HEAD:test-webkitpy when run in the Tools/Scripts directory). I consider both of these things rather badly broken and would love it if the SCM abstraction hid them.

I feel like I too have seen git barf on absolute paths.  Which may have been part of how we ended up doing stuff relative to checkout_root for git.  It's sad that SVN and Git differ in their absolute path handling.  I agree SCM should hide this.  Sadly SCM needs to also hide the differences between Git and SVN as much as possible.

> I'll check this as well tomorrow. My comments were not meant in any way as a criticism of your code, but rather the underlying stupidity of Git, Python, and the Mac filesystem (I mean, honestly, couldn't the porters have just returned /private/var/tmp? And what's with the ridiculous generated directory names?).

Oh, criticize away! :)  scm.py has a lot of problems.  I keep meaning to break it up into a module (splitting out the 3 classes into at least 3 files) and finally make the unit tests fast enough so we can run them all the time.

Thanks again for your efforts.