[kwy404]

Root cause: in get_instances_with_configs, the try block covers both AndroidDevice(serial) and ad.load_config(c). When the constructor itself raises for a device configured with required: False, the except branch calls ad.log.exception(...), but ad was never assigned for that device. For the first config this raises UnboundLocalError instead of skipping the optional device; for later configs it logs the error under the previous device.

Fix: log the skip with logging.exception and the serial, so it no longer depends on ad. One line in mobly/controllers/android_device.py.

Test: added test_get_instances_with_configs_skips_optional_device_on_init_error, where the first (optional) device raises in the constructor and the second succeeds. Before the fix it fails with UnboundLocalError: cannot access local variable 'ad'; after the fix it passes and only the second device is returned.

[xpconanfan]

LGTM, thanks for the fix.

Verified: the diagnosis is correct — ad is unbound when AndroidDevice(serial) itself raises, so an optional device's failure turned into UnboundLocalError (first config) or got logged under the previous device (later configs). Switching to logging.exception(..., serial) removes the dependency on ad while keeping the serial in the message. The new test fails on main with UnboundLocalError and passes with the change; the surrounding get_instances_with_configs tests still pass locally.

[xpconanfan]

The one-line fix is correct, and I confirmed the new test fails on main with UnboundLocalError and passes with the change. Before merging, please tighten the test so it actually pins the behavior this PR fixes:

1. Assert the log, not just the return value. assertEqual(ads, [mock_ad]) would also pass if the except branch were a bare continue. The point of the fix is that the skip is logged by serial without touching ad, so wrap the call in with self.assertLogs(level='ERROR') as cm: and assert 'Skipping optional device 1 due to error.' is in cm.output.

2. Cover the "later config" case described in the PR. With the failing device first, the misattribution path (error logged under the previous device's ad.log) is never exercised. Add a case with configs = [{'serial': '1'}, {'serial': '2', 'required': False}] where the second constructor raises, and assert mock_ad.log.exception.assert_not_called() plus the module-level log above.

3. Nit: key the failure on the serial rather than call position, so the test doesn't silently depend on construction order:

   def _make_ad(serial):
     if serial == '1':
       raise Exception('Something went wrong.')
     return mock_ad
   mock_ad_class.side_effect = _make_ad

Thanks for the fix!

[kwy404]

Thanks for the careful review @xpconanfan, all three are done in 22c5f0c:

1. Both tests now wrap the call in self.assertLogs(level='ERROR') and assert that Skipping optional device <serial> due to error. is in cm.output, so a bare continue would fail them.
2. Added test_get_instances_with_configs_skips_later_optional_device_on_init_error with [{'serial': '1'}, {'serial': '2', 'required': False}], where the second constructor raises. It asserts the module level log for serial 2 and mock_ad.log.exception.assert_not_called(), which covers the misattribution path.
3. The mock now fails by serial through _make_ad(serial) instead of call position.

Both tests fail on main and pass with the fix.

[xpconanfan]

LGTM, thanks for addressing the review comments!