Bug 66228 – REGRESSION (NRWT): Leaks Viewer can't load leaks from test runs that used NRWT

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

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.

[Adam Roben (:aroben)]

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)/

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

I've reverted back to ORWT for the Leaks bot for now: http://trac.webkit.org/changeset/93046

[Adam Roben (:aroben)]

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.

[Eric Seidel (no email)]

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.

[Adam Roben (:aroben)]

(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.

[Eric Seidel (no email)]

Created attachment 105512 [details]
Patch

[Eric Seidel (no email)]

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.

[Dirk Pranke]

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?

[Eric Seidel (no email)]

(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.

[Dirk Pranke]

(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.

[Eric Seidel (no email)]

(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?

[Dirk Pranke]

(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'.

[Darin Adler]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Created attachment 105995 [details]
Fix typo from Darin's review

[Dirk Pranke]

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

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Created attachment 106009 [details]
Removed extra entry from ChangeLog and removed spurious import glob statement

[Dirk Pranke]

(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.

[Eric Seidel (no email)]

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.

[Dirk Pranke]

(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 ...

[Dirk Pranke]

http://stackoverflow.com/questions/445910/create-regex-from-glob-expression probably gets us close, although one commenter actually suggests fnmatch().

[Eric Seidel (no email)]

fnmatch is what glob uses under the covers. :)

[Eric Seidel (no email)]

The problem with fnmatch is that it's platform-specific (according to the docs).  Which we don't want for a Mock.

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

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.