Rebaseline script should update only existing files by default.

Rebaseline script should update only existing files by default.

[bungeman, 2012-12-06 23:31:21]

Brian was doing some gm rebaselining today and the need to wait for everything to download just to svn st | grep '^A' | cut -c9- | xargs rm svn st | grep '^!' | cut -c9- | xargs svn revert to remove the adds seemed a bit silly. This change makes it so that by default only existing files are fetched, which can be overridden by '--get-new-images'. It also makes the output somewhat more clear and indicates when an add is attempted.

[Stephen White, 2012-12-07 15:45:16]

On 2012/12/06 23:31:21, bungeman wrote: > Brian was doing some gm rebaselining today and the need to wait for everything > to download just to > > svn st | grep '^A' | cut -c9- | xargs rm > svn st | grep '^!' | cut -c9- | xargs svn revert > > to remove the adds seemed a bit silly. This change makes it so that by default > only existing files are fetched, which can be overridden by '--get-new-images'. > It also makes the output somewhat more clear and indicates when an add is > attempted. I would prefer that anyone rebaselining a test will be enhancing the test coverage by default, not leaving it half-baselined (this is also how WebKit's rebaseline tools work). So I would prefer that this operate the other way by default, i.e., --existing-files-only or --only-existing-files or whatever.

[bsalomon, 2012-12-07 15:48:08]

When I ran the script and then ran svn stat all the unbaselined files showed up as ?s not As. It does look like the existing script would add them, though. Also I noticed that it is missing msaa16 and mesa configs. Should those be added? They aren't run on all the platforms so you get a bunch of failed to download msgs.

[Stephen White, 2012-12-07 15:48:46]

https://codereview.appspot.com/6912044/diff/1/tools/rebaseline.py File tools/rebaseline.py (right): https://codereview.appspot.com/6912044/diff/1/tools/rebaseline.py#newcode77 tools/rebaseline.py:77: print '[Couldn\'t fetch]' Yeah, the output is pretty ugly. Here's what I'd like to see instead: testname: added. testname: modified. testname: no change. testname: missing. WDYT?

[Stephen White, 2012-12-07 15:55:24]

On 2012/12/07 15:48:08, bsalomon wrote: > When I ran the script and then ran svn stat all the unbaselined files showed up > as ?s not As. It does look like the existing script would add them, though. Hmm, yeah. They should be added. I'll spin up an SVN checkout to see what's up. > Also I noticed that it is missing msaa16 and mesa configs. Should those be > added? I didn't add the msaa16 configs since they tend to be flaky. I suppose we could make msaa16 an exception to the "always add new files" rule, but is there really value in having flaky tests? It seems like they're just filling up the SVN repo for no good reason. We could definitely add mesa, though -- I didn't know it had baselines. > They aren't run on all the platforms so you get a bunch of failed to > download msgs. If we clean up the output (see above), it shouldn't be too bad to have missing configs (that big ugly message is left from when I was debugging the script, should go away).

[bsalomon, 2012-12-07 15:59:31]

On 2012/12/07 15:55:24, Stephen White wrote: > On 2012/12/07 15:48:08, bsalomon wrote: > > When I ran the script and then ran svn stat all the unbaselined files showed > up > > as ?s not As. It does look like the existing script would add them, though. > > Hmm, yeah. They should be added. I'll spin up an SVN checkout to see what's > up. > > > Also I noticed that it is missing msaa16 and mesa configs. Should those be > > added? > > I didn't add the msaa16 configs since they tend to be flaky. I suppose we could > make msaa16 an exception to the "always add new files" rule, but is there really > value in having flaky tests? It seems like they're just filling up the SVN repo > for no good reason. They're flaky on the Macs but I think they are OK on the other bots for the most part. Also, none of the Android devices run MSAA16. > > We could definitely add mesa, though -- I didn't know it had baselines. Mesa is run just on the macs. > > > They aren't run on all the platforms so you get a bunch of failed to > > download msgs. > > If we clean up the output (see above), it shouldn't be too bad to have missing > configs (that big ugly message is left from when I was debugging the script, > should go away). SGTM.

[Stephen White, 2012-12-07 16:05:34]

On 2012/12/07 15:59:31, bsalomon wrote: > On 2012/12/07 15:55:24, Stephen White wrote: > > On 2012/12/07 15:48:08, bsalomon wrote: > > > When I ran the script and then ran svn stat all the unbaselined files showed > > up > > > as ?s not As. It does look like the existing script would add them, though. > > > > Hmm, yeah. They should be added. I'll spin up an SVN checkout to see what's > > up. > > > > > Also I noticed that it is missing msaa16 and mesa configs. Should those be > > > added? > > > > I didn't add the msaa16 configs since they tend to be flaky. I suppose we > could > > make msaa16 an exception to the "always add new files" rule, but is there > really > > value in having flaky tests? It seems like they're just filling up the SVN > repo > > for no good reason. > > They're flaky on the Macs but I think they are OK on the other bots for the most > part. Also, none of the Android devices run MSAA16. Another thing I've been meaning to add is a --config parameter, so you could rebaseline only that config. So it wouldn't do msaa16 by default, but if you wanted to rebaseline them you could do: rebaseline.py --config msaa16 --existing-files-only foo bar baz > > We could definitely add mesa, though -- I didn't know it had baselines. > > Mesa is run just on the macs. OK. We could make the script smarter about configs, and have each platform have a list of configs. Then we wouldn't hit the network unnecessarily for things we know it won't find. (We could also make it smarter and use HTTP keepalive to fetch all the files, so it doesn't pay TCP slow-start for each file fetch. That would probably need some more python learnin' on my part, though.) > > > They aren't run on all the platforms so you get a bunch of failed to > > > download msgs. > > > > If we clean up the output (see above), it shouldn't be too bad to have missing > > configs (that big ugly message is left from when I was debugging the script, > > should go away). > > SGTM.

[bsalomon, 2012-12-07 16:08:22]

Pipe-dream: store all the possible configs and per platform exclusions in a file that is read both by GM and by baselining tools. On Fri, Dec 7, 2012 at 11:05 AM, <senorblanco@chromium.org> wrote: > On 2012/12/07 15:59:31, bsalomon wrote: > >> On 2012/12/07 15:55:24, Stephen White wrote: >> > On 2012/12/07 15:48:08, bsalomon wrote: >> > > When I ran the script and then ran svn stat all the unbaselined >> > files showed > >> > up >> > > as ?s not As. It does look like the existing script would add >> > them, though. > >> > >> > Hmm, yeah. They should be added. I'll spin up an SVN checkout to >> > see what's > >> > up. >> > >> > > Also I noticed that it is missing msaa16 and mesa configs. Should >> > those be > >> > > added? >> > >> > I didn't add the msaa16 configs since they tend to be flaky. I >> > suppose we > >> could >> > make msaa16 an exception to the "always add new files" rule, but is >> > there > >> really >> > value in having flaky tests? It seems like they're just filling up >> > the SVN > >> repo >> > for no good reason. >> > > They're flaky on the Macs but I think they are OK on the other bots >> > for the most > >> part. Also, none of the Android devices run MSAA16. >> > > Another thing I've been meaning to add is a --config parameter, so you > could rebaseline only that config. So it wouldn't do msaa16 by default, > but if you wanted to rebaseline them you could do: > > rebaseline.py --config msaa16 --existing-files-only foo bar baz > > > > We could definitely add mesa, though -- I didn't know it had >> > baselines. > > Mesa is run just on the macs. >> > > OK. We could make the script smarter about configs, and have each > platform have a list of configs. Then we wouldn't hit the network > unnecessarily for things we know it won't find. > > (We could also make it smarter and use HTTP keepalive to fetch all the > files, so it doesn't pay TCP slow-start for each file fetch. That would > probably need some more python learnin' on my part, though.) > > > > > They aren't run on all the platforms so you get a bunch of failed >> > to > >> > > download msgs. >> > >> > If we clean up the output (see above), it shouldn't be too bad to >> > have missing > >> > configs (that big ugly message is left from when I was debugging the >> > script, > >> > should go away). >> > > SGTM. >> > > > > https://codereview.appspot.**com/6912044/<https://codereview.appspot.com/6912... > 

[bungeman, 2012-12-07 16:08:24]

On 2012/12/07 15:45:16, Stephen White wrote: > On 2012/12/06 23:31:21, bungeman wrote: > > Brian was doing some gm rebaselining today and the need to wait for everything > > to download just to > > > > svn st | grep '^A' | cut -c9- | xargs rm > > svn st | grep '^!' | cut -c9- | xargs svn revert > > > > to remove the adds seemed a bit silly. This change makes it so that by default > > only existing files are fetched, which can be overridden by > '--get-new-images'. > > It also makes the output somewhat more clear and indicates when an add is > > attempted. > > I would prefer that anyone rebaselining a test will be enhancing the test > coverage by default, not leaving it half-baselined (this is also how WebKit's > rebaseline tools work). So I would prefer that this operate the other way by > default, i.e., > > --existing-files-only > > or > --only-existing-files > > or whatever. I disagree with the idealism, and WebKit is also wrong. If we want to add new baselines we do that once (after that they get updated when we rebaseline). The majority of the time, however, we made a change and just want to verify nothing broke and make the bots go green again. No one ever wants to be 'enhancing the test coverage by default' because that means evaluating new images to see if they should be new baselines, and we frankly never do that. So I say make the common case easy.

[bungeman, 2012-12-07 16:17:08]

On 2012/12/07 15:48:46, Stephen White wrote: > https://codereview.appspot.com/6912044/diff/1/tools/rebaseline.py > File tools/rebaseline.py (right): > > https://codereview.appspot.com/6912044/diff/1/tools/rebaseline.py#newcode77 > tools/rebaseline.py:77: print '[Couldn\'t fetch]' > Yeah, the output is pretty ugly. Here's what I'd like to see instead: > > testname: added. > testname: modified. > testname: no change. > testname: missing. > > WDYT? That's what svn st is for? I see the output of this tool as just a pretty progress bar so that I know something is happening. I don't see any need for it to try to tell me something non-athoratatively when svn st can tell me better. I only added the '+' for adds for testing really, I don't care if it's there.

[Stephen White, 2012-12-07 16:21:21]

On 2012/12/07 15:48:08, bsalomon wrote: > When I ran the script and then ran svn stat all the unbaselined files showed up > as ?s not As. It does look like the existing script would add them, though. Hmm... I ran senorblanco-linux:~/src/skia-whole-enchilada-svn/gm-expected[]% ../trunk/tools/rebaseline.py aaclip And see only A's: A base-macmini/aaclip_8888.png A base-macmini/aaclip_565.png A base-macmini/aaclip_pdf.png A base-macmini-lion-float/aaclip_8888.png A base-macmini-lion-float/aaclip_565.png Is that not what you're seeing? (Note that the script only really works on Linux and Mac, doesn't really work on Windows).

[bsalomon, 2012-12-07 16:22:06]

On 2012/12/07 16:21:21, Stephen White wrote: > On 2012/12/07 15:48:08, bsalomon wrote: > > When I ran the script and then ran svn stat all the unbaselined files showed > up > > as ?s not As. It does look like the existing script would add them, though. > > Hmm... I ran > > senorblanco-linux:~/src/skia-whole-enchilada-svn/gm-expected[]% > ../trunk/tools/rebaseline.py aaclip > > And see only A's: > > A base-macmini/aaclip_8888.png > A base-macmini/aaclip_565.png > A base-macmini/aaclip_pdf.png > A base-macmini-lion-float/aaclip_8888.png > A base-macmini-lion-float/aaclip_565.png > > Is that not what you're seeing? > > (Note that the script only really works on Linux and Mac, doesn't really work on > Windows). I'll test again but I had ?s on both mac and linux.

[Stephen White, 2012-12-07 16:24:55]

On 2012/12/07 16:17:08, bungeman wrote: > On 2012/12/07 15:48:46, Stephen White wrote: > > https://codereview.appspot.com/6912044/diff/1/tools/rebaseline.py > > File tools/rebaseline.py (right): > > > > https://codereview.appspot.com/6912044/diff/1/tools/rebaseline.py#newcode77 > > tools/rebaseline.py:77: print '[Couldn\'t fetch]' > > Yeah, the output is pretty ugly. Here's what I'd like to see instead: > > > > testname: added. > > testname: modified. > > testname: no change. > > testname: missing. > > > > WDYT? > > That's what svn st is for? I see the output of this tool as just a pretty > progress bar so that I know something is happening. I don't see any need for it > to try to tell me something non-athoratatively when svn st can tell me better. True. I suppose the only thing it can tell you over and above svn is the difference between "no change" and "missing". I'm fine if that's all it does. I wonder if we should have it run "svn status" or "git status" when done?

[bsalomon, 2012-12-07 16:26:37]

On 2012/12/07 16:22:06, bsalomon wrote: > On 2012/12/07 16:21:21, Stephen White wrote: > > On 2012/12/07 15:48:08, bsalomon wrote: > > > When I ran the script and then ran svn stat all the unbaselined files showed > > up > > > as ?s not As. It does look like the existing script would add them, though. > > > > Hmm... I ran > > > > senorblanco-linux:~/src/skia-whole-enchilada-svn/gm-expected[]% > > ../trunk/tools/rebaseline.py aaclip > > > > And see only A's: > > > > A base-macmini/aaclip_8888.png > > A base-macmini/aaclip_565.png > > A base-macmini/aaclip_pdf.png > > A base-macmini-lion-float/aaclip_8888.png > > A base-macmini-lion-float/aaclip_565.png > > > > Is that not what you're seeing? > > > > (Note that the script only really works on Linux and Mac, doesn't really work > on > > Windows). > > I'll test again but I had ?s on both mac and linux. Still ?s. Maybe the difference is that I have directly checked out gm-expected and not the entire svn repo.

[Stephen White, 2012-12-07 16:33:15]

On 2012/12/07 16:26:37, bsalomon wrote: > On 2012/12/07 16:22:06, bsalomon wrote: > > On 2012/12/07 16:21:21, Stephen White wrote: > > > On 2012/12/07 15:48:08, bsalomon wrote: > > > > When I ran the script and then ran svn stat all the unbaselined files > showed > > > up > > > > as ?s not As. It does look like the existing script would add them, > though. > > > > > > Hmm... I ran > > > > > > senorblanco-linux:~/src/skia-whole-enchilada-svn/gm-expected[]% > > > ../trunk/tools/rebaseline.py aaclip > > > > > > And see only A's: > > > > > > A base-macmini/aaclip_8888.png > > > A base-macmini/aaclip_565.png > > > A base-macmini/aaclip_pdf.png > > > A base-macmini-lion-float/aaclip_8888.png > > > A base-macmini-lion-float/aaclip_565.png > > > > > > Is that not what you're seeing? > > > > > > (Note that the script only really works on Linux and Mac, doesn't really > work > > on > > > Windows). > > > > I'll test again but I had ?s on both mac and linux. > > Still ?s. Maybe the difference is that I have directly checked out gm-expected > and not the entire svn repo. Ahh! That's it. My svn/git detection only looks at the parent directory. Will fix.