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

Fix and improve tests for Python != 3.7 #95

Merged
merged 1 commit into from
Aug 26, 2019

Conversation

Jamim
Copy link
Contributor

@Jamim Jamim commented Aug 20, 2019

Hello everyone,

I believe it would be nice to have tests on CI not only for Python 3.7, but for all supported Python versions.

These changes:

Cheers!

- '3.7'
- 'pypy3.5'
- '3.8-dev'
Copy link
Contributor

@carlosalberto carlosalberto Aug 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. However, we really don't need to have 3.8-dev, and specially the nightly one, IHMO (even with the allow_failures flag) ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's any benefit IHMO. dev can change relatively a lot. I don't think there's any value in having it here honestly (unless we were married to some internal or prototype feature of it).

Anyway, I wouldn't totally be against this, so I will leave others give their opinion ;)

Copy link
Contributor Author

@Jamim Jamim Aug 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Python 3.8 is almost there, so it makes sense to have tests for it.

In addition, I believe that it's always better to know about any incompatibilities sooner rather than later.
Especially if running the tests costs nothing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does make builds slower. But I have no strong opinion on dev either. I also believe that nightly is too much, things can still change there so if we fix an error in nightly and then nightly changes to make the fix unnecessary, we wasted time.

Copy link
Contributor Author

@Jamim Jamim Aug 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @carlosalberto and @Oberon00,
I've removed nightly for now.

@a-feld
Copy link
Member

a-feld commented Aug 20, 2019

Hi @Jamim thanks for your PR! 🎉

Python 3.4 has reached end of life and I would imagine OpenTelemetry would not add support for this version of Python since this software will be released after the end-of-life.

That being said, ultimately the maintainers have control over the project direction regarding Python 3.4 😄

@@ -62,13 +62,18 @@
"""

from contextlib import contextmanager
import typing
from typing import Callable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC we decided to import modules, i.e. typing, rather than importing individual members (not sure we documented it anywhere, btw).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please reconsider this.

_TRACER_FACTORY: typing.Optional[
    typing.Callable[[typing.Type[Tracer]], typing.Optional[Tracer]]] = None

looks really terrible.

Copy link
Contributor

@carlosalberto carlosalberto Aug 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One could also argue the same with importing a different class per line ;) I've never liked this in Java, for example. You can also always put each parameter in a new line. Oh well, a matter of taste, I guess.

Maybe @reyang happens to remember whether we had some style guidelines?

Copy link
Contributor Author

@Jamim Jamim Aug 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One could also argue the same with importing a different class per line ;)

We have to say "thank you" to isort for that.

Copy link
Contributor Author

@Jamim Jamim Aug 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to propose to change preferences for isort to make imports less ugly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was documented in #36. I'm not entirely sure but if isort doesn't complain it should be fine? My opinion was that we definitely must not have from x import y with the same x multiple times (like this does), and we definitely must not have an import x, y (multiple modules in the same import). If we want to discuss other aspects, we should not do this here but reopen #36 or create a new issue. I agree that both possibilities that isort allows now are suboptimal (multiple from import for the same module is something I definitely don't want though).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated isort settings in c9821ec.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related PR: #109

@Jamim
Copy link
Contributor Author

Jamim commented Aug 20, 2019

Hello @a-feld,

Thank you for the feedback.
Actually, I know that 3.4 was EOLed.
To be honest, I always prefer to not support EOLed Python versions, but it was already there, so I kept it.
I can remove support for Python 3.4 if everyone agrees.

@carlosalberto
Copy link
Contributor

Python 3.4 has reached end of life and I would imagine OpenTelemetry would not add support for this version of Python since this software will be released after the end-of-life.

I'm wondering about this too. Initially we decided to support this, but I started having second thoughts on this ;)

Worth discussing it again in #25 (and maybe in the SIG this Thursday as well).

@Oberon00 Oberon00 changed the title Fix and improve tests Fix and improve tests for Python != 3.7 Aug 21, 2019
@@ -108,7 +108,9 @@ def validate_response(self, response, error=None):
self.span_context_manager.__exit__.assert_not_called()
self.assertEqual(value, b"*")
except StopIteration:
self.span_context_manager.__exit__.assert_called()
self.span_context_manager.__exit__.assert_called_with(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too bad we need this change. 🙁 Which Python version requires it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need it for 3.5 and 3.4.
The assert_called method was introduced in 3.6.

tox.ini Outdated Show resolved Hide resolved
@@ -62,13 +62,18 @@
"""

from contextlib import contextmanager
import typing
from typing import Callable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was documented in #36. I'm not entirely sure but if isort doesn't complain it should be fine? My opinion was that we definitely must not have from x import y with the same x multiple times (like this does), and we definitely must not have an import x, y (multiple modules in the same import). If we want to discuss other aspects, we should not do this here but reopen #36 or create a new issue. I agree that both possibilities that isort allows now are suboptimal (multiple from import for the same module is something I definitely don't want though).

opentelemetry-api/src/opentelemetry/trace/__init__.py Outdated Show resolved Hide resolved
@@ -145,7 +145,9 @@ def __repr__(self):
__all__ = ['Context']


Context: typing.Optional[BaseRuntimeContext]
Context = ( # pylint: disable=invalid-name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me wonder if we shouldn't relax the pylint rule for "constants" (maybe even to .*) since there are so many things it demands UPPER_CASE for which are not really constants. Then this could be a single line without the awkward parens.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's done. Please review b05f347.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related PR: #108

@Oberon00
Copy link
Member

@Jamim thank you for taking care of this! 🎉

@Oberon00
Copy link
Member

Oberon00 commented Aug 21, 2019

Please avoid force-pushing over already reviewed commits, that makes it hard to see what changed since the last review (force-pushing before merging to clean up the commit history is totally fine though).
I just noticed you can click the force-pushed link to see the diff 😄 Still it breaks the Diff UI's "changes since you last reviewed" feature.

@Jamim
Copy link
Contributor Author

Jamim commented Aug 21, 2019

Hello @carlosalberto and @Oberon00,
Please re-review this PR and consider resolving or updating of the discussions.
Thanks!

@carlosalberto
Copy link
Contributor

If we want to discuss other aspects, we should not do this here but reopen #36 or create a new issue.

I think we should have this PR without the isort and related style changes - or wait till we re-review #36 (re-open it maybe?)

@Jamim
Copy link
Contributor Author

Jamim commented Aug 21, 2019

@carlosalberto I can extract b05f347 and c9821ec into a separate PR.
How do you think?

@Oberon00
Copy link
Member

LGTM now, but the style changes need further discussion (though I like them personally). So splitting this up is probably for the best.

@Jamim
Copy link
Contributor Author

Jamim commented Aug 22, 2019

Hello @carlosalberto and @Oberon00,

I've excluded b05f347 and c9821ec for now.
Once this PR is merged, I'll create a corresponding PR including changes from those commits.
Please let me now if any additional changes are required.

Thanks!

@Oberon00
Copy link
Member

The only thing that keeps me from approving now is the long list of from typing imports in loader.py. If you want my approval now, change it back to a single import typing, otherwise you'll have to wait with this PR until the import discussion is resolved (maybe in today's SIG meeting?).

I believe it would be nice to have
tests on CI not only for Python 3.7,
but for all supported Python versions.

These changes:

 - fix compatibility with Python 3.5 and 3.4

 - add tests for various Python versions on CI

 - allow running tests for any branches
@Jamim
Copy link
Contributor Author

Jamim commented Aug 22, 2019

@Oberon00:
If you want my approval now, change it back to a single import typing

It's done.

Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Several questions were spawned by this PR (Python 3.4 support, import style, pylint constant naming rule) but they can be discussed independently later.

@Oberon00 Oberon00 mentioned this pull request Aug 22, 2019
@Jamim
Copy link
Contributor Author

Jamim commented Aug 23, 2019

Hello @carlosalberto,
What can I do to gain your approval? Please let me know.
Thanks!

@carlosalberto carlosalberto self-requested a review August 23, 2019 14:01
@carlosalberto
Copy link
Contributor

@Jamim remind me ;) (it's approved now)

@Jamim
Copy link
Contributor Author

Jamim commented Aug 24, 2019

Hi everyone,
Since this PR is approved and there is no additional feedback, can it be merged now?

@Oberon00 Oberon00 merged commit 2bcb228 into open-telemetry:master Aug 26, 2019
@Oberon00
Copy link
Member

Done! Thx for the reminder!

@Jamim Jamim deleted the fix/tests branch August 26, 2019 17:43
Jamim added a commit to Jamim/opentelemetry-python that referenced this pull request Aug 30, 2019
These changes follow up the "Fix and improve tests for Python != 3.7" PR.

The multi_line_output was already set to 3 in the
"Add initial black formatting" PR, so after rebasing to master
this commit contains only comment that describes a magic number
from the isort configuration file.

Corresponding PR:

 - open-telemetry#109

Related discussions:

 - open-telemetry#95 (comment)

 - open-telemetry#95 (comment)
Oberon00 pushed a commit that referenced this pull request Sep 5, 2019
These changes follow up the "Fix and improve tests for Python != 3.7" PR.

The multi_line_output was already set to 3 in the
"Add initial black formatting" PR, so after rebasing to master
this commit contains only comment that describes a magic number
from the isort configuration file.

Corresponding PR:

 - #109

Related discussions:

 - #95 (comment)

 - #95 (comment)
@c24t c24t mentioned this pull request Feb 11, 2020
NathanielRN pushed a commit to NathanielRN/opentelemetry-python-contrib-1 that referenced this pull request Oct 20, 2020
These changes follow up the "Fix and improve tests for Python != 3.7" PR.

The multi_line_output was already set to 3 in the
"Add initial black formatting" PR, so after rebasing to master
this commit contains only comment that describes a magic number
from the isort configuration file.

Corresponding PR:

 - open-telemetry/opentelemetry-python#109

Related discussions:

 - open-telemetry/opentelemetry-python#95 (comment)

 - open-telemetry/opentelemetry-python#95 (comment)
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants