Hi Geoff, some more comments.

[7]

+if os.path.dirname(os.path.abspath(sys.argv[0])) == script_dir:
+    sys.path.insert(0, "./")
+    data_path = os.path.abspath(os.path.join(script_dir, os.path.pardir))
+    print data_path
+else:
+    from landscape.lib.warning import hide_warnings
+    hide_warnings()
+    data_path = None
+    print "B"

I suspect the print statements here are a leftover.

[8]

+        # self.select_landscape_hosting()

This line is commented.

[9]

+        if data_path is None:
+            self._ui_path = os.path.join(
+                controller.data_path, "ui",
+                ClientSettingsDialog.GLADE_FILE)

As discussed on IRC, data_path is for run-time data, better to put the glade file in the same directory as the python module.

[10]

+class SettingsConfiguration(LandscapeSetupConfiguration):
+    required_options = []

As mentioned, instead of subclassing it'd be better to drop the required "url" option from the base class and add a sensible default for it instead (the url of hosted).

[11]

+        with self._lock:

This will make the module fail to import from the tests on Python 2.5.

[12]

+        self._configuration.load([])

As mentioned, this should be passed argv, so we can have:

./scripts/landscape-client-settings-ui -c root-client.conf

(root-client.conf is the default file that you would use for testing a branch against a local server, like ./scripts/landscape-client -c root-client.conf)

[13]

+    @server_host_name.setter
+    def server_host_name(self, value):

I'd rename it to _set_server_host_name, to clearness and to make lint tools happy (there's are redefinition warning). Same for:

+    @account_name.setter
+    def account_name(self, value):

[14]

+        self._configuration.load([])
+        self._load_data_from_config()

This is minor, but I general I find __init__ methods that end up reading files or querying databases or similar "big" side effects a bit odd, and they often make the class tad less testable (indeed you have to use 2 test case classes to, with one to test the empty config file case). They should usually just set some attributes.

What do you think of moving the loading logic to a "load" method (or some other appropriate name), that you can drive from the tests? So we could have a single test like this:


class ConfigControllerTest(LandscapeTest):

    def setUp(self):
        super(ConfigControllerTest, self).setUp()
        config = "[client]"
        config += "data_path = /var/lib/landscape/client\n"
        config += "http_proxy = http://proxy.localdomain:3192\n"
        config += "tags = a_tag\n"
        config += "url = https://landscape.canonical.com/message-system\n"
        config += "account_name = foo\n"
        config += "registration_password = bar\n"
        config += "computer_title = baz\n"
        config += "https_proxy = https://proxy.localdomain:6192\n"
        config += "ping_url = http://landscape.canonical.com/ping\n"
        self.config_filename = self.makeFile(config)

        class MySettingsConfiguration(SettingsConfiguration):
            default_config_filenames = [self.config_filename]

        self.config = MySettingsConfiguration()
        self.controller = ConfigController(self.config)

    def test_init(self):
        """
        Test that when we create a controller it has initial state read in
        directly from the configuration file.
        """
        self.controller.load()
        self.assertEqual(self.controller.data_path, "/var/lib/landscape/client/")
        # ... etc

    def test_defaulting(self):
        """
        Test we set the correct values when switching between hosted and
        dedicated.
        """
        self.makeFile("", path=self.config_filename)  # This will just empty the config file
        self.controller.load()
        self.assertEqual(self.controller.account_name, None)
        # ... etc