Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow unit tests to pass if pytz is installed. #1706

Merged
merged 1 commit into from
Apr 8, 2016
Merged

Allow unit tests to pass if pytz is installed. #1706

merged 1 commit into from
Apr 8, 2016

Conversation

rimey
Copy link
Contributor

@rimey rimey commented Apr 8, 2016

I'm assuming the intent of test_module_property() is
to check that gcloud._helpers.UTC is defined and has
the right sort of value. It was failing if pytz
happened to be installed.

Note that the current test environment does not
install pytz. The gcloud package uses pytz if it is
available but does not depend on it.

@rimey rimey added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. api: core labels Apr 8, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 8, 2016
@tseaver tseaver self-assigned this Apr 8, 2016
@tseaver
Copy link
Contributor

tseaver commented Apr 8, 2016

Thanks for the patch! ISTM that a better fix for the test would be to have it try to import pytz and DTRT depending on success. E.g.:

    def test_module_property(self):
        from gcloud import _helpers as MUT
        klass = self._getTargetClass()
        try:
            import pytz
        except ImportError:
            self.assertTrue(isinstance(MUT.UTC, klass))
        else:
            self.assertTrue(MUT.UTC is pytz.UTC)

I'm assuming the intent of test_module_property() is
to check that gcloud._helpers.UTC is defined and has
the right sort of value. It was failing if pytz
happened to be installed.

Note that the current test environment does _not_
install pytz. The gcloud package uses pytz if it is
available but does not depend on it.

As per a suggestion from @tseaver, we test gcloud._helpers.UTC
against pytz.UTC if pytz is available.
@rimey
Copy link
Contributor Author

rimey commented Apr 8, 2016

Done. PTAL.

@tseaver tseaver merged commit 3cf7d03 into googleapis:master Apr 8, 2016
@tseaver
Copy link
Contributor

tseaver commented Apr 8, 2016

Thanks very much!

@dhermes
Copy link
Contributor

dhermes commented Apr 8, 2016

Why do we no cover both branches?

@rimey
Copy link
Contributor Author

rimey commented Apr 8, 2016

Only one is executed in any given test environment. Did I understand your
question correctly?

On Fri, Apr 8, 2016 at 11:48 AM, Danny Hermes notifications@github.com
wrote:

Why do we no cover both branches?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#1706 (comment)

@dhermes
Copy link
Contributor

dhermes commented Apr 8, 2016

But the relevant coverage environment, i.e. the one that get's reported on our repo (from coveralls.io) use the tox cover environment. We should have that branch without a no cover pragma.

@rimey
Copy link
Contributor Author

rimey commented Apr 8, 2016

I guess the idea is that the test code will do the right thing in any
reasonable environment, while the coverage pragmas on that test code will
be correct for the tox cover environment? I can send another pull request.

On Fri, Apr 8, 2016 at 11:55 AM, Danny Hermes notifications@github.com
wrote:

But the relevant coverage environment, i.e. the one that get's reported on
our repo (from coveralls.io) use the tox cover environment. We should
have that branch without a no cover pragma.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#1706 (comment)

@dhermes
Copy link
Contributor

dhermes commented Apr 8, 2016

Thanks for sending the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: core cla: yes This human has signed the Contributor License Agreement. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants