Bug 38692 – rebaseline-chromium-webkit-tests crashes if nothing is built

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

rebaseline-chromium-webkit-tests crashes if nothing is built

[Dirk Pranke]

Created attachment 55298 [details]
Patch

[Dirk Pranke]

*** Bug 38405 has been marked as a duplicate of this bug. ***

[Victor Wang]

LGTM

[Eric Seidel (no email)]

Comment on attachment 55298 [details]
Patch

I dislike this fallback logic.  I think we should respect set-webkit-configuration and then possibly have auto-fallback from whatever that is defaulted to.  No other script has this strange fallback stuff.

That said, this change didn't make it any worse.

This change however lacks any unit testing.  It should be simple to test this.

[Dirk Pranke]

(In reply to comment #4)
> This change however lacks any unit testing.  It should be simple to test this.

So, I'm not an expert on mock objects, but I don't see an easy way to inject anything into this in order to be able to test it. Suggestions? Do I need to refactor the code out to actually pass a port object into main()?

[Eric Seidel (no email)]

I would make the "figure out what configuration to use" code into a separate method.  Then its inputs and outputs should be clearly defined, and it should be possible to Mock the necessary inputs and assert the expected outputs.

[Dirk Pranke]

Created attachment 55449 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 55449 [details]
Patch

I dislike the way we do the testing for the existance of the image_diff binary.  However, you've not changed the previous code, and it's now tested, so this patch is a huge win! :)

I think that the design error here is that configuration is a property of a port.  It's information provided to a port, but it is not itself a property of said port.  Since it is, it makes use create a port just to test a configuration.  Waht we really want to do is have some port instance which we can then ask questions pertaining to a configuration, and then store that configuration either in the port or separate.

Thanks for all the hard work!

[Eric Seidel (no email)]

Attachment 55449 [details] was posted by a committer and has review+, assigning to Dirk Pranke for commit.

[Dirk Pranke]

(In reply to comment #8)
> (From update of attachment 55449 [details])
> I dislike the way we do the testing for the existance of the image_diff binary.  However, you've not changed the previous code, and it's now tested, so this patch is a huge win! :)
>

I agree that this should probably just use the default configuration logic and not try to be smart.

 
> I think that the design error here is that configuration is a property of a port.  It's information provided to a port, but it is not itself a property of said port.  Since it is, it makes use create a port just to test a configuration.  Waht we really want to do is have some port instance which we can then ask questions pertaining to a configuration, and then store that configuration either in the port or separate.

I think there's two different issues here. First, this code is hard(-er) to test because it's calling factory methods to attempt to find a port that can meet the script's needs (namely, a working image_diff). The script looks at both the debug and release configurations of the default port -- and that logic would be fixed by your change -- but it could just as easily look at (say) the webkit mac and chromium mac ports, and you'd be left with the same problem of needing to create multiple ports.

The second issue is whether or not a port object should expose the concept of configurations or not. My initial design very much did not want to do this - the only place the caller should (arguably) be aware of the concept of configurations is when passing arguments to the factory method. This makes all of the other methods simpler (one less concept and one less optional argument). Apart from this particular example, this design has worked well so far.

You are right that we could instead have the concept that a port supports N different configurations and expose the configuration as a selector  to various methods. Apart from this use case, there doesn't really seem to be any need for this, and I think we would both agree that even this use case is weird.

So, I don't think I would consider the port/configuration design choice an error. Does that make sense?
> 
> Thanks for all the hard work!

[Dirk Pranke]

Comment on attachment 55449 [details]
Patch

Clearing flags on attachment: 55449

Committed r59091: <http://trac.webkit.org/changeset/59091>

[Dirk Pranke]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

FWIW, there is already at least one example of Port methods being configuration agnostic as you and I discussed over chat:
http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_linux.py#L147

If we ended up separating the idea of Port and Configuration we could have a wrapper object which knew how to answer questions from some Port given some Configuration.

In webkit's perl code Configuration actually contains 32bit vs. 64bit as well information as well.  Eventually maybe we'll expand that concept to include other stored build/config flags.

[Eric Seidel (no email)]

Another part of the webkit-patch code also builds both --debug and --release.  It doesn't do that using any sort of configuration object, but it could eventually. Not sure we really need to shave that yak yet.

[Dirk Pranke]

(In reply to comment #13)
> FWIW, there is already at least one example of Port methods being configuration agnostic as you and I discussed over chat:
> http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_linux.py#L147
>

True, but that method is (intended to be) private and it implements logic that should probably also be removed in favor of a default-configuration-type approach.