Bug 46278 – NRWT doesn't respect config set with set-webkit-configuration

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Mihai Parparita]

Even though I ran set-webkit-configuration --debug a while back (and build-webkit --chromium respects it), NRWT kept looking for the DRT binary in the Release directory until I passed an explicit --debug flag to it.

I suspect http://trac.webkit.org/changeset/68008 (unintentionally?) triggered this behavior change.

[Dirk Pranke]

Odd ... I thought I had a test for this. Looking into it now; it's certainly possible that that change broke this.

[Mihai Parparita]

+Adam, since he reviewed r68008.

There was a change to chromium.py which is responsible for this:

http://trac.webkit.org/changeset/68008/trunk/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py

Was there a reason why ChromiumPort now sets the default configuration to Release in it constructor? There is code that runs later (_set_up_derived_options in run_webkit_tests.py) which would do the similar initialization:

    if not options.configuration:
        options.configuration = port_obj.default_configuration()

[Mihai Parparita]

(In reply to comment #2)
> Was there a reason why ChromiumPort now sets the default configuration to Release in it constructor? 

I now see that that code was always there, just duplicated in Mac/Linux/Windows subclasses of ChromiumPort. However, 

if options and not hasattr(options, 'configuration'): 
    options.configuration = 'Release'

Whereas now it's more aggressive:

if (options and (not hasattr(options, 'configuration') or
                 options.configuration is None)):
    options.configuration = 'Release'

options.configuration is defined by parse_args in run_webkit_tests.py, it's just set to None.

Any objection to reverting the options.configuration check to the way it was before?

[Dirk Pranke]

No objection. This code was accidentally broken.

I am in the middle of fixing it properly and writing better unit tests so that it doesn't break again. This turns out to involve a fair amount of yak-shaving, but I should have a patch this evening.

[Mihai Parparita]

(In reply to comment #4)
> I am in the middle of fixing it properly and writing better unit tests so that it doesn't break again. This turns out to involve a fair amount of yak-shaving, but I should have a patch this evening.

I was looking into the "right" way of fixing it this afternoon, and it seemed like having the ChromiumPort constructor call self.default_configuration() instead of hardcoding 'Release' would be better, but I wasn't sure what the implications of that would be (for starters, the unit tests didn't pass, since they didn't mock out the configuration reading so were getting it from the filesystem).

[Mihai Parparita]

Created attachment 68690 [details]
Patch

[Mihai Parparita]

+Tony, since I see he rolled back Dirk's quick fix since it broke NRWT on the canary bots (http://trac.webkit.org/changeset/68234). What exactly broke, and how can I make sure my patch doesn't break things too?

[Tony Chang]

(In reply to comment #7)
> +Tony, since I see he rolled back Dirk's quick fix since it broke NRWT on the canary bots (http://trac.webkit.org/changeset/68234). What exactly broke, and how can I make sure my patch doesn't break things too?

It failed because of:
  import pdb
  pdb.set_trace()

Which put the interpreter into interactive mode.

[WebKit Commit Bot]

Comment on attachment 68690 [details]
Patch

Clearing flags on attachment: 68690

Committed r68268: <http://trac.webkit.org/changeset/68268>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

(In reply to comment #6)
> Created an attachment (id=68690) [details]
> Patch

Yup, that'll work, although there's a lot more scaffolding than I would hope to have and it still doesn't test all the way through to make sure that every port is actually hitting the config file to pull the default (i.e., if ChromiumPort accidentally (?) defines a different default_configuration() we won't notice it in these tests).

So, I'll keep hacking on the "right" fix but thanks for taking care of this for now.

-- Dirk

[Andrey Kosyakov]

This change apparently broke nacl_ui_tests in chromium:

http://build.chromium.org/buildbot/waterfall/builders/NACL%20Tests/builds/17999

[Mihai Parparita]

(In reply to comment #12)
> This change apparently broke nacl_ui_tests in chromium:
> 
> http://build.chromium.org/buildbot/waterfall/builders/NACL%20Tests/builds/17999

Thanks for letting me know.  I've put up a fix at bug 46602.

[Mihai Parparita]

(In reply to comment #13)
> Thanks for letting me know.  I've put up a fix at bug 46602.

Landed as http://trac.webkit.org/changeset/68365.