Code review comment for lp:~alecu/ubuntuone-client/proxy-tunnel-process

Revision history for this message
Alejandro J. Cura (alecu) wrote :

> A very stupid comments but why doing:
>
> if len(proxies) and proxies[0].type() != QNetworkProxy.NoProxy:
> return True
>
> When you an do:
>
> return len(proxies) and proxies[0].type() != QNetworkProxy.NoProxy
>
> same for:
>
> if settings:
> return True
>
> In IsProxyEnabledTestCase I think you can merge the tests cases, for example:
>
> def _assert_enabled(self, platform):
> """Assert that the proxy is enabled."""
> self.patch(tunnel_server.sys, "platform", platform)
> ret = tunnel_server.is_proxy_enabled(SAMPLE_HOST,
> str(SAMPLE_PORT))
> self.assertTrue(ret, "Proxy is enabled.")
>
> def test_platform_linux_enabled(self):
> """Tests for the linux platform with proxies enabled."""
> self.patch(tunnel_server.gsettings, "get_proxy_settings",
> lambda: FAKE_SETTINGS)
> self._assert_enabled('linux3')
>
> def test_platform_other_enabled(self):
> """Tests for any other platform with proxies enabled."""
> fake_netproxfact = FakeNetworkProxyFactoryClass(True)
> self.patch(tunnel_server, "QNetworkProxyFactory",
> fake_netproxfact)
> self._assert_enabled('windows 1.0')
>
> Same for the disabled one.

Refactored all those tests as suggested.

> I think it would be a good idea to use sys.exit in main specially in the case
> were there is no proxy.

It's a normal condition for this process to exit if there's no proxy configured, so I think that it would be redundant to use sys.exit in this case.

Thanks for the review.

« Back to merge proposal