[BEAM-3713] Add pytest for unit tests #9756

udim · 2019-10-09T23:50:22Z

This is #7949 without IT support.

This does not replace regular precommit/postcommit/tox environments.
Pytest will replace nose once we can verify all tests are collected by pytest and blocking bugs are solved (see BEAM-3713).

Runs unit tests using pytest
- tox: tox -e py27-gcp-pytest,py36-pytest,etc.
- gradle: ../../gradlew pythonPreCommitPytest
- github PR phrase: run python_pytest precommit
Tests run in parallel (still single-threaded but on multiple worker
processes).
- no_xdist marker used for tests that don't work the xdist plugin.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

Post-Commit Tests Status (on master branch)

Lang	SDK	Apex	Dataflow	Gearpump	Samza	Spark
Go		---	---	---	---
Java
Python		---		---	---
XLang	---	---	---	---	---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website
Non-portable
Portable	---		---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

udim · 2019-10-09T23:50:34Z

run seed job

udim · 2019-10-10T00:19:27Z

run seed job

udim · 2019-10-10T00:27:17Z

R: @chadrik

This should be tested with a "run seed job", which should succeed. "run python_pytest precommit" will then launch a pytest-based precommit job (without ITs), but it's not critical that this job passes at this stage.

chadrik · 2019-10-10T16:16:14Z

This looks great. What's the plan for deploying it? Do you plan to merge it as is to get it in front of more users for testing, or will you replace nose with pytest before the merge?

udim · 2019-10-10T18:25:41Z

I will first merge the PR as is with separate targets for nose and pytest. Once we are satisfied that we have no missing tests and people have had the chance to try it, we'll switch over to pytest by default.

…

On Thu, Oct 10, 2019, 09:16 Chad Dombrova ***@***.***> wrote: This looks great. What's the plan for deploying it? Do you plan to merge it as is to get it in front of more users for testing, or will you replace nose with pytest before the merge? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9756?email_source=notifications&email_token=AAA7FTZ3FUXXGSQBLHP7KOLQN5IOXA5CNFSM4I7GBNFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEA45P7Y#issuecomment-540661759>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA7FT4SQAAYJNUQCX4QDI3QN5IOXANCNFSM4I7GBNFA> .

chadrik · 2019-10-10T21:22:52Z

Sounds good.

…

On Thu, Oct 10, 2019 at 2:26 PM Udi Meiri ***@***.***> wrote: I will first merge the PR as is with separate targets for nose and pytest. Once we are satisfied that we have no missing tests and people have had the chance to try it, we'll switch over to pytest by default. On Thu, Oct 10, 2019, 09:16 Chad Dombrova ***@***.***> wrote: > This looks great. What's the plan for deploying it? Do you plan to merge > it as is to get it in front of more users for testing, or will you replace > nose with pytest before the merge? > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > < #9756?email_source=notifications&email_token=AAA7FTZ3FUXXGSQBLHP7KOLQN5IOXA5CNFSM4I7GBNFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEA45P7Y#issuecomment-540661759 >, > or unsubscribe > < https://github.com/notifications/unsubscribe-auth/AAA7FT4SQAAYJNUQCX4QDI3QN5IOXANCNFSM4I7GBNFA > > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9756?email_source=notifications&email_token=AAAPOE3N7DQIDJ5CXAWJUITQN5XUNA5CNFSM4I7GBNFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEA5J34Q#issuecomment-540712434>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAPOE7ZW6LREEVESBKBRNLQN5XUNANCNFSM4I7GBNFA> .

chadrik · 2019-10-11T02:06:45Z

sdks/python/apache_beam/runners/dataflow/dataflow_runner_test.py

@@ -228,6 +229,8 @@ def test_biqquery_read_streaming_fail(self):
                                 r'source is not currently available'):
      p.run()

+  @pytest.mark.skipif(sys.version_info >= (3, 7),
+                      reason='TODO(BEAM-8095): Segfaults in Python 3.7')


hmmm... this only segfaults when using pytest? what happens if you disable xdist?

I changed it to no_xdist. It still segfaults when using xdist or when attempting to run under a debugger in PyCharm.

Regarding problems with this test, see: https://issues.apache.org/jira/browse/BEAM-8397?focusedCommentId=16958087&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16958087

udim · 2019-10-14T17:22:20Z

@chadrik PTAL
CC/R: @markflyhigh, @chamikaramj

chadrik · 2019-10-14T17:38:24Z

sdks/python/conftest.py

+if sys.version_info < (3,):
+  collect_ignore_glob.append('*_py3.py')
+for minor in [5, 6, 7, 8, 9]:
+  if sys.version_info < (3, minor):
+    collect_ignore_glob.append('*_py3%d.py' % minor)


How about something like this:

MAX_SUPPORTED_PYTHON_VERSION = (3, 8) # See pytest.ini for main collection rules. collect_ignore_glob = [] if sys.version_info < (3,): collect_ignore_glob.append('*_py3.py') else: for minor in range(sys.version_info.minor +1, MAX_SUPPORTED_PYTHON_VERSION[1] + 1): collect_ignore_glob.append('*_py3%d.py' % minor)

oh, btw, what did you think of this suggestion?

IDK. It's more verbose but I don't have a preference.

Ideally, I'd like a single source of truth for the list of Python supported versions (python_requires in setup.py is such a source but it's hard to parse).

IDK. It's more verbose but I don't have a preference.

I think it'll be easier to maintain long term: just update the max support python version.

But maintainability aside, it seems a bit odd to test for our lower bound in a loop when we know precisely what the lower bound is: sys.version_info.minor +1

Ideally, I'd like a single source of truth for the list of Python supported versions (python_requires in setup.py is such a source but it's hard to parse).

true.

But maintainability aside, it seems a bit odd to test for our lower bound in a loop when we know precisely what the lower bound is: sys.version_info.minor +1

You're right.

chadrik · 2019-10-21T03:47:45Z

I had two thoughts related to this today, as I struggled trying to diagnose some failed tests on jenkins.

it would be great if you could make the gradle tasks run serially, since we're already using xdist to parallelize the work. That will make the logs easier to read since it won't interleave different tox tasks together.
I added a PR to enable color in tox: WIP: tox: allow apps that support color to use it #9840. Hopefully this will work with pytest, because I think it will add a lot of value (unbelievably, nose does not support color out of the box).

udim · 2019-10-21T18:25:50Z

I had two thoughts related to this today, as I struggled trying to diagnose some failed tests on jenkins.

it would be great if you could make the gradle tasks run serially, since we're already using xdist to parallelize the work. That will make the logs easier to read since it won't interleave different tox tasks together.
Yes that makes sense, the logs already make my browser slow down to a crawl (Reduce the verbosity of local_job_service_main #9813). Let's do that in another PR though.

I added a PR to enable color in tox: WIP: tox: allow apps that support color to use it #9840. Hopefully this will work with pytest, because I think it will add a lot of value (unbelievably, nose does not support color out of the box).

Colors already work for me with tox and pytest. Let's get this PR merged so you can try it out yourself. :)

chadrik · 2019-10-21T19:46:18Z

Colors already work for me with tox and pytest.

Great. I've been digging into my problem with colored mypy output problem and I think it's a tty issue. I think mypy needs a mechanism to force colors even when a tty is not present.

it would be great if you could make the gradle tasks run serially, since we're already using xdist to parallelize the work. That will make the logs easier to read since it won't interleave different tox tasks together.

Yes that makes sense, the logs already make my browser slow down to a crawl (#9813). Let's do that in another PR though.

I've also been thinking about investigating using tox's support fo parallel execution instead of gradle: https://tox.readthedocs.io/en/latest/example/basic.html#parallel-mode

Let's get this PR merged so you can try it out yourself. :)

Looks good to me!

This is apache#7949 without IT support. - Runs unit tests using pytest - tox: `tox -e py27-gcp-pytest,py36-pytest,etc.` - gradle: `../../gradlew pythonPreCommitPytest` - github PR phrase: `run python_pytest precommit` - Tests run in parallel (still single-threaded but on multiple worker processes). - no_xdist marker used for tests that don't work the xdist plugin. - Allows specifying test module in tox cmd line. Example: ```sh tox -e py27-pytest apache_beam.transforms.window_test ```

... instead of completely skipping it for Python 3.7.

udim · 2019-10-21T20:39:04Z

PTAL

chadrik · 2019-11-01T17:44:53Z

Looks good. I say we merge this once the tests are confirmed passing.

udim · 2019-11-01T19:36:05Z

Run Java PreCommit

udim · 2019-11-01T19:36:26Z

Run CommunityMetrics PreCommit

udim · 2019-11-01T19:38:26Z

Run Python PreCommit

udim · 2019-11-01T22:00:13Z

Run Python PreCommit

udim · 2019-11-04T17:22:10Z

Merging since tests pass. Will address BEAM-8095 TODO in a subsequent PR. I believe it has been fixed in #9881.

This is apache#7949 without IT support. - Runs unit tests using pytest - tox: `tox -e py27-gcp-pytest,py36-pytest,etc.` - gradle: `../../gradlew pythonPreCommitPytest` - github PR phrase: `run python_pytest precommit` - Tests run in parallel (still single-threaded but on multiple worker processes). - no_xdist marker used for tests that don't work the xdist plugin. - Allows specifying test module in tox cmd line. Example: ```sh tox -e py27-pytest apache_beam.transforms.window_test ```

chadrik reviewed Oct 11, 2019

View reviewed changes

chadrik reviewed Oct 14, 2019

View reviewed changes

udim added 4 commits October 21, 2019 13:19

Fix duplicate variable name

f2cb621

Mark test_remote_runner_display_data as noxdist

64e1642

... instead of completely skipping it for Python 3.7.

Addressed review comment

636f1ae

udim force-pushed the pytest-unittests branch from b0e79de to 636f1ae Compare October 21, 2019 20:37

udim merged commit a6936c8 into apache:master Nov 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-3713] Add pytest for unit tests #9756

[BEAM-3713] Add pytest for unit tests #9756

udim commented Oct 9, 2019

udim commented Oct 9, 2019

udim commented Oct 10, 2019

udim commented Oct 10, 2019

chadrik commented Oct 10, 2019

udim commented Oct 10, 2019 via email

chadrik commented Oct 10, 2019 via email

chadrik Oct 11, 2019

udim Oct 11, 2019

tvalentyn Oct 23, 2019 •

edited

udim commented Oct 14, 2019

chadrik Oct 14, 2019

chadrik Oct 21, 2019

udim Oct 21, 2019

chadrik Oct 21, 2019

udim Oct 21, 2019

chadrik commented Oct 21, 2019

udim commented Oct 21, 2019

chadrik commented Oct 21, 2019 •

edited

udim commented Oct 21, 2019

chadrik commented Nov 1, 2019

udim commented Nov 1, 2019

udim commented Nov 1, 2019

udim commented Nov 1, 2019

udim commented Nov 1, 2019

udim commented Nov 4, 2019

[BEAM-3713] Add pytest for unit tests #9756

[BEAM-3713] Add pytest for unit tests #9756

Conversation

udim commented Oct 9, 2019

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

udim commented Oct 9, 2019

udim commented Oct 10, 2019

udim commented Oct 10, 2019

chadrik commented Oct 10, 2019

udim commented Oct 10, 2019 via email

chadrik commented Oct 10, 2019 via email

chadrik Oct 11, 2019

Choose a reason for hiding this comment

udim Oct 11, 2019

Choose a reason for hiding this comment

tvalentyn Oct 23, 2019 • edited

Choose a reason for hiding this comment

udim commented Oct 14, 2019

chadrik Oct 14, 2019

Choose a reason for hiding this comment

chadrik Oct 21, 2019

Choose a reason for hiding this comment

udim Oct 21, 2019

Choose a reason for hiding this comment

chadrik Oct 21, 2019

Choose a reason for hiding this comment

udim Oct 21, 2019

Choose a reason for hiding this comment

chadrik commented Oct 21, 2019

udim commented Oct 21, 2019

chadrik commented Oct 21, 2019 • edited

udim commented Oct 21, 2019

chadrik commented Nov 1, 2019

udim commented Nov 1, 2019

udim commented Nov 1, 2019

udim commented Nov 1, 2019

udim commented Nov 1, 2019

udim commented Nov 4, 2019

tvalentyn Oct 23, 2019 •

edited

chadrik commented Oct 21, 2019 •

edited