Bug 66228 - REGRESSION (NRWT): Leaks Viewer can't load leaks from test runs that used NRWT
Summary: REGRESSION (NRWT): Leaks Viewer can't load leaks from test runs that used NRWT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Eric Seidel (no email)
URL: http://build.webkit.org/LeaksViewer/
Keywords: Regression
Depends on:
Blocks: 34984
  Show dependency treegraph
 
Reported: 2011-08-15 08:04 PDT by Adam Roben (:aroben)
Modified: 2011-09-01 19:56 PDT (History)
4 users (show)

See Also:


Attachments
Patch (6.65 KB, patch)
2011-08-29 12:51 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Fix typo from Darin's review (5.84 KB, patch)
2011-09-01 11:20 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Removed extra entry from ChangeLog and removed spurious import glob statement (6.35 KB, patch)
2011-09-01 12:28 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2011-08-15 08:04:32 PDT
To reproduce:

1. Go to http://build.webkit.org/LeaksViewer/?url=%2Fresults%2FSnowLeopard%20Intel%20Leaks%2Fr93040%20%2818578%29%2F

It just says "Loading…" forever.
Comment 1 Adam Roben (:aroben) 2011-08-15 08:05:48 PDT
This is at least partly due to the leaks files being named differently in ORWT and NRWT. For comparison:

ORWT: http://build.webkit.org/old-results/SnowLeopard%20Intel%20Leaks/r92628%20(18480)/
NRWT: http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r93040%20(18578)/
Comment 2 Eric Seidel (no email) 2011-08-15 10:00:59 PDT
I'm happy to name them however you'd like, but I can't (because we use multiple processes) have a single increasing counter for the file numbers.

http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/mac.py#L103

Is where the name is generated.  That code is per-worker-instance.  (Well, it exists in both the worker and the master process), so we don't have any way to coordinate names except through the file system.  We could look at what files exist on disk, but then we'd have to look between the workers or we could end up with a race condition.
Comment 3 Eric Seidel (no email) 2011-08-15 10:22:18 PDT
I've reverted back to ORWT for the Leaks bot for now: http://trac.webkit.org/changeset/93046
Comment 4 Adam Roben (:aroben) 2011-08-15 10:42:06 PDT
We don't necessarily have to change the names. We could teach Leaks Viewer how to interpret the NRWT-style names.

One advantage of ORWT's serial test running + monotonically increasing leaks file numbers is that it's possible to figure out which leaks came from which tests. With NRWT it seems like that will be considerably harder. That's probably worth a separate bug.
Comment 5 Eric Seidel (no email) 2011-08-29 12:20:22 PDT
How do we best move forward with this?  Seems the simplest thing is to make the new LeaksViewer understand the new filename pattern.  It's impossible (and meaningless) in a parallel environment for us to name the leaks files with increasing numbers in order of creation.
Comment 6 Adam Roben (:aroben) 2011-08-29 12:25:38 PDT
(In reply to comment #5)
> Seems the simplest thing is to make the new LeaksViewer understand the new filename pattern.

That seems like a fine solution to me.
Comment 7 Eric Seidel (no email) 2011-08-29 12:51:05 PDT
Created attachment 105512 [details]
Patch
Comment 8 Eric Seidel (no email) 2011-08-29 12:52:22 PDT
Dirk:  If you have suggestions on how to fix the integration tests, I'm all ears.  But it appears that for most tests the skipped lists are not applied (or userscripts aren't skipped for the Mac port?), thus now that our glob() implementation works, we correctly end up trying to run this "userscripts" test, which then causes badness.
Comment 9 Dirk Pranke 2011-08-29 13:02:34 PDT
Comment on attachment 105512 [details]
Patch

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

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:146
> +        wildcard_index = glob_string.find('*')

Maybe we should rewrite this to the equivalent regexp, i.e., ([^\/]*), and just do a regexp match? Does that fix the test?
Comment 10 Eric Seidel (no email) 2011-08-29 13:06:37 PDT
(In reply to comment #9)
> (From update of attachment 105512 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105512&action=review
> 
> > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:146
> > +        wildcard_index = glob_string.find('*')
> 
> Maybe we should rewrite this to the equivalent regexp, i.e., ([^\/]*), and just do a regexp match? Does that fix the test?

I can try. I suspect that that would simply break the test in just the same way.
Comment 11 Dirk Pranke 2011-08-29 13:08:29 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 105512 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=105512&action=review
> > 
> > > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:146
> > > +        wildcard_index = glob_string.find('*')
> > 
> > Maybe we should rewrite this to the equivalent regexp, i.e., ([^\/]*), and just do a regexp match? Does that fix the test?
> 
> I can try. I suspect that that would simply break the test in just the same way.

I think the test should now work, because the problem before with the "broken" implementation was that it wasn't recognizing directory separators as delimiters, and hence it was doing something a real glob wouldn't do. We should be bringing this closer to the real glob's semantics this way. If not, I'm guessing some variation on my suggestion should work, at least.
Comment 12 Eric Seidel (no email) 2011-08-29 13:23:07 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (From update of attachment 105512 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=105512&action=review
> > > 
> > > > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:146
> > > > +        wildcard_index = glob_string.find('*')
> > > 
> > > Maybe we should rewrite this to the equivalent regexp, i.e., ([^\/]*), and just do a regexp match? Does that fix the test?
> > 
> > I can try. I suspect that that would simply break the test in just the same way.
> 
> I think the test should now work, because the problem before with the "broken" implementation was that it wasn't recognizing directory separators as delimiters, and hence it was doing something a real glob wouldn't do. We should be bringing this closer to the real glob's semantics this way. If not, I'm guessing some variation on my suggestion should work, at least.

I'm not sure I follow what you're saying?  I haven't tried the regexp approach.  I was looking at http://docs.python.org/library/fnmatch.html, but I worry it will do the wrong thing on windows.

Could you explain more?
Comment 13 Dirk Pranke 2011-08-29 13:38:36 PDT
(In reply to comment #12)
> I'm not sure I follow what you're saying?  I haven't tried the regexp approach.  I was looking at http://docs.python.org/library/fnmatch.html, but I worry it will do the wrong thing on windows.
> 
> Could you explain more?

Since we're actually implementing glob, you don't actually want fnmatch semantics, you want glob semantics (http://docs.python.org/library/glob.html#module-glob). This means that 'f*' should match 'foo.html' and 'foobar/' but not 'foobar/baz.html'.
Comment 14 Darin Adler 2011-08-29 17:51:51 PDT
Comment on attachment 105512 [details]
Patch

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

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:145
> +        # FIXME: This only handles the simpliest of wildcard matches.

Typo: simplest
Comment 15 Eric Seidel (no email) 2011-09-01 11:05:58 PDT
The unittest failure was caused by me fixing the typo, and had nothing to do with the glob changes.

Made that commit in http://trac.webkit.org/changeset/94316
Comment 16 Eric Seidel (no email) 2011-09-01 11:20:13 PDT
Created attachment 105995 [details]
Fix typo from Darin's review
Comment 17 Dirk Pranke 2011-09-01 12:07:17 PDT
Comment on attachment 105995 [details]
Fix typo from Darin's review

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

> Tools/ChangeLog:19
> +        * Scripts/webkitpy/layout_tests/port/test.py:

Nit. Doesn't look like you're actually changing port/test.py?

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:154
> +        # We could use fnmatch.fnmatch, but that might not do the right thing on windows.

Can you try my re.match suggestion instead ? If that passes the tests, I'd prefer we do that, as the code is likely much cleaner than this (and will handle more cases to boot).
Comment 18 Eric Seidel (no email) 2011-09-01 12:14:02 PDT
Comment on attachment 105995 [details]
Fix typo from Darin's review

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

>> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:154
>> +        # We could use fnmatch.fnmatch, but that might not do the right thing on windows.
> 
> Can you try my re.match suggestion instead ? If that passes the tests, I'd prefer we do that, as the code is likely much cleaner than this (and will handle more cases to boot).

Specifically you're suggesting that I do this:

pattern = re.compile(glob_string.replace('*', '\w+'))

Is that correct?
Comment 19 Eric Seidel (no email) 2011-09-01 12:26:33 PDT
Comment on attachment 105995 [details]
Fix typo from Darin's review

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

>>> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:154
>>> +        # We could use fnmatch.fnmatch, but that might not do the right thing on windows.
>> 
>> Can you try my re.match suggestion instead ? If that passes the tests, I'd prefer we do that, as the code is likely much cleaner than this (and will handle more cases to boot).
> 
> Specifically you're suggesting that I do this:
> 
> pattern = re.compile(glob_string.replace('*', '\w+'))
> 
> Is that correct?

I've tried both that, and re.compile(re.escape(glob_string).replace('\*', '\w+')) and failed to get it to pass the tests.

I'd like to stick with this simpler solution unless you have a better suggestion.
Comment 20 Eric Seidel (no email) 2011-09-01 12:28:55 PDT
Created attachment 106009 [details]
Removed extra entry from ChangeLog and removed spurious import glob statement
Comment 21 Dirk Pranke 2011-09-01 13:20:25 PDT
(In reply to comment #19)
> (From update of attachment 105995 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105995&action=review
> 
> >>> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:154
> >>> +        # We could use fnmatch.fnmatch, but that might not do the right thing on windows.
> >> 
> >> Can you try my re.match suggestion instead ? If that passes the tests, I'd prefer we do that, as the code is likely much cleaner than this (and will handle more cases to boot).
> > 
> > Specifically you're suggesting that I do this:
> > 
> > pattern = re.compile(glob_string.replace('*', '\w+'))
> > 
> > Is that correct?
> 
> I've tried both that, and re.compile(re.escape(glob_string).replace('\*', '\w+')) and failed to get it to pass the tests.
> 
> I'd like to stick with this simpler solution unless you have a better suggestion.

No, sorry, I meant pattern = re.compile(glob_string.replace('*', '[^\/]*')). It would need to be '*', not '+', and  \w wouldn't handle files w/ spaces in them (and probably a bunch of other legal filename characters).

At any rate, thanks for trying. Up to you if you want to try my suggestion again (I'll try it out later while I'm waiting for a compile or something). R+ otherwise.
Comment 22 Eric Seidel (no email) 2011-09-01 13:28:46 PDT
Comment on attachment 106009 [details]
Removed extra entry from ChangeLog and removed spurious import glob statement

Thank you dirk.  I think we'll still have trouble escaping other possible regexp characters in the filename.  We can revisit this later if needed.
Comment 23 Dirk Pranke 2011-09-01 13:34:02 PDT
(In reply to comment #22)
> (From update of attachment 106009 [details])
> Thank you dirk.  I think we'll still have trouble escaping other possible regexp characters in the filename.  We can revisit this later if needed.

Ah, right, yeah '.' would probably screw things up and hence need to be escaped ...
Comment 24 Dirk Pranke 2011-09-01 13:35:51 PDT
http://stackoverflow.com/questions/445910/create-regex-from-glob-expression probably gets us close, although one commenter actually suggests fnmatch().
Comment 25 Eric Seidel (no email) 2011-09-01 13:43:58 PDT
fnmatch is what glob uses under the covers. :)
Comment 26 Eric Seidel (no email) 2011-09-01 13:44:17 PDT
The problem with fnmatch is that it's platform-specific (according to the docs).  Which we don't want for a Mock.
Comment 27 WebKit Review Bot 2011-09-01 14:28:58 PDT
Comment on attachment 106009 [details]
Removed extra entry from ChangeLog and removed spurious import glob statement

Clearing flags on attachment: 106009

Committed r94345: <http://trac.webkit.org/changeset/94345>
Comment 28 WebKit Review Bot 2011-09-01 14:29:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Dirk Pranke 2011-09-01 19:56:02 PDT
Okay, fixing glob turned out to be slightly more complicated, but it revealed that we actually had (at least) two different bugs. See bug 67462.