Clean up unittest deprecations and warnings #2130

xoob · 2017-05-25T20:18:59Z

This PR cleans up a number of warnings and notices in the tests. Detailed descriptions are in the commit messages.

Motivation and Context

Needed to get aquatinted with the test infrastructure. Aside from that, general maintenance and keeping things neat.

Have you tested this? If so, how?

Tested locally with tox -e py36-nonhdfs and tox -e py27-nonhdfs.

Fixes getpcmd on macOS by calling 'pgrep' instead of reading '/proc', which is not available. This fixes two failures in lock_test. Also fixes an issue where UTF-8 process could not be hashed in Python 2.6.

Fixes "DeprecationWarning: Please use assertEqual instead."

Fixes noisy warnings during tests by using assertWarns() added in Python 3.

Fixes a number of deprecation warnings regarding configuration: test_config_summary_limit (execution_summary_test.ExecutionSummaryTest) ... DeprecationWarning: Configuration [execution_summary] summary_length (with dashes) should be avoided. Please use underscores. test_disable_emails (worker_test.WorkerEmailTest) ... DeprecationWarning: The use of the configuration [core] email-type is deprecated. Please use [email] format test_task_limit_exceeded (worker_test.TaskLimitTest) ... DeprecationWarning: The use of the configuration [core] worker-task-limit is deprecated. Please use [worker] task_limit test_complete_error (worker_test.WorkerEmailTest) ... DeprecationWarning: Configuration [email] force_send (with dashes) should be avoided. Please use underscores. test_scheduler_with_config (scheduler_test.SchedulerIoTest) ... DeprecationWarning: Configuration [scheduler] worker_disconnect_delay (with dashes) should be avoided. Please use underscores. test_sendgrid (notifications_test.TestNotificationDispatcher) ... DeprecationWarning: Configuration [email] force_send (with dashes) should be avoided. Please use underscores. DeprecationWarning: The use of the configuration [email] type is deprecated. Please use [email] method Call notificaions.send_email_smtp with fixture parameters with smtp_without_tls set to False ... DeprecationWarning: The use of the configuration [core] smtp_host is deprecated. Please use [smtp] host DeprecationWarning: The use of the configuration [core] smtp_local_hostname is deprecated. Please use [smtp] local_hostname ... Call notificaions.send_email_sendgrid with fixture parameters ... DeprecationWarning: The use of the configuration [email] SENDGRID_USERNAME is deprecated. Please use [sendgrid] username DeprecationWarning: The use of the configuration [email] SENDGRID_PASSWORD is deprecated. Please use [sendgrid] password

mention-bot · 2017-05-25T20:19:01Z

@xoob, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Tarrasch, @erikbern and @daveFNbuck to be potential reviewers.

xoob · 2017-05-25T20:39:21Z

luigi/lock.py

-    else:
-        cmd_hash = my_cmd
-
+    cmd_hash = my_cmd.encode('utf8')


Hmm, now this causes breakage on Linux, py27: UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 4: ordinal not in range(128).

Would this change to line 52 be appropriate?

-return fh.read().replace('\0', ' ').rstrip() +return fh.read().replace('\0', ' ').decode('utf8').rstrip()

Tarrasch · 2017-05-26T08:16:40Z

Thank you so much!!!

Can you just fix the flake8 error? (also check why py27 fails)

Tarrasch · 2017-06-30T10:08:18Z

@xoob, this is a really good PR which I would like to merge, can you just fix the flake8 error?

xoob · 2017-06-30T16:05:26Z

I'll take a shot at it this weekend.

erikbern · 2017-07-01T13:01:46Z

looks awesome!

Fixes an issue where utf8 cmdline args where not consistently decoded in py27 on Linux. See also https://github.com/spotify/luigi/pull/2130/files#r118579419

xoob · 2017-07-03T19:18:10Z

@Tarrasch @erikbern All tests and flake8 now pass.

Unrelated to the changes in this PR, the py27-cdh and py36-cdh suites time out after 50 minutes. This seems to happen on master too.

Tarrasch · 2017-07-03T20:10:24Z

The commit message quality was quite mixed, but overall I thought it might be useful to keep the whole commit history as some commits were particularly useful to be able to introspect in isolation. So did a rebase-merge here. :)

Thanks a lot for this contribution. This greatly increases the code quality in luigi!

xoob · 2017-07-03T20:24:30Z

@Tarrasch Which commit messages worked for you and how would you improve the other ones?

Tarrasch · 2017-07-03T20:33:54Z

@xoob good question! These are my opinion and they are by no means better than anyone else's:

I really liked the first one. As it has a clear description and useful details in the body
Actually I definitely liked most of your commits
I found one to have >20 lines of text in body. Which might feel not so scroll friendly in some views, and I doubt anybody is going to read all that, and it is maybe obvious from the actual code change.
I prefer flake8 fixes to not be separate, but I do it myself too, because it's so hard to amend them in later.
The last 3 commits seem to say the same thing and should probably have been only 1 (unless I'm mistaken).

Are any of these thoughts useful as feedback? :)

xoob · 2017-07-03T20:39:06Z

@Tarrasch Very useful, thanks. I could have amended and force-pushed the last three commits (and possibly the flake8 changes too), which I'll do next time. Appreciate the feedback.

Tarrasch · 2017-07-03T20:43:58Z

I could have amended and force-pushed the last three commits

Not everyone is so responsive to iterate on their commits. Thanks for your cooperation. ^^

xoob added 8 commits May 25, 2017 20:19

Fix getpcmd on macOS

c676395

Fixes getpcmd on macOS by calling 'pgrep' instead of reading '/proc', which is not available. This fixes two failures in lock_test. Also fixes an issue where UTF-8 process could not be hashed in Python 2.6.

Fix DeprecationWarning in tests

4eb3962

Fixes "DeprecationWarning: Please use assertEqual instead."

Fix don't skip S3 test cases fixed upstream

9b54d7a

Fix test silence warnings

69acdce

Fixes noisy warnings during tests by using assertWarns() added in Python 3.

Refactor rename MockFile -> MockTarget

532d31a

Refactor test to use assertTrue

542f6c6

Fix tests to use new config format

741ae84

Reformat flake8 complaints

5ff2316

xoob commented May 25, 2017

View reviewed changes

xoob added 3 commits July 3, 2017 10:40

Fix decode to utf8 when reading /proc/*/cmdline

8c2f946

Fixes an issue where utf8 cmdline args where not consistently decoded in py27 on Linux. See also https://github.com/spotify/luigi/pull/2130/files#r118579419

Fix decode to utf8 when reading /proc/*/cmdline

ec24d5b

Fix decode to utf8 when reading /proc/*/cmdline

b896587

Tarrasch merged commit 24dfe0f into spotify:master Jul 3, 2017

Tarrasch mentioned this pull request Jul 3, 2017

Travis: Remove minicluster builds #2141

Merged

xoob mentioned this pull request Jul 3, 2017

WIP: Restore gcloud tests in Travis #2169

Closed

xoob deleted the fix-tests branch July 3, 2017 20:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up unittest deprecations and warnings #2130

Clean up unittest deprecations and warnings #2130

xoob commented May 25, 2017

mention-bot commented May 25, 2017

xoob May 25, 2017

Tarrasch commented May 26, 2017

Tarrasch commented Jun 30, 2017

xoob commented Jun 30, 2017 via email •

edited

Loading

erikbern commented Jul 1, 2017

xoob commented Jul 3, 2017

Tarrasch commented Jul 3, 2017

xoob commented Jul 3, 2017

Tarrasch commented Jul 3, 2017

xoob commented Jul 3, 2017

Tarrasch commented Jul 3, 2017 •

edited

Loading

Clean up unittest deprecations and warnings #2130

Clean up unittest deprecations and warnings #2130

Conversation

xoob commented May 25, 2017

Motivation and Context

Have you tested this? If so, how?

mention-bot commented May 25, 2017

xoob May 25, 2017

Choose a reason for hiding this comment

Tarrasch commented May 26, 2017

Tarrasch commented Jun 30, 2017

xoob commented Jun 30, 2017 via email • edited Loading

erikbern commented Jul 1, 2017

xoob commented Jul 3, 2017

Tarrasch commented Jul 3, 2017

xoob commented Jul 3, 2017

Tarrasch commented Jul 3, 2017

xoob commented Jul 3, 2017

Tarrasch commented Jul 3, 2017 • edited Loading

xoob commented Jun 30, 2017 via email •

edited

Loading

Tarrasch commented Jul 3, 2017 •

edited

Loading