Codecov Report

Merging #934 into master will decrease coverage by 0.02%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master     #934      +/-   ##
==========================================
- Coverage    91.8%   91.78%   -0.03%     
==========================================
  Files         103      103              
  Lines        4040     4042       +2     
  Branches      638      639       +1     
==========================================
+ Hits         3709     3710       +1     
- Misses        193      194       +1     
  Partials      138      138

Hi,

Thanks for your contribution. I would like to see some tests for the new method (selecting none, one and multiple). This will also hit the coverage target.
After that looks good to me.

@Eldinnie I'm not sure, what do you mean about selecting none, one and multiple. So I run the following codes to test:

Codes:

import time
from telegram.ext import Updater

TOKEN = "412548814:AAHhkKLF5rStuI1A_T5k9hb0-1qyiUQ-TY0" #fake token

def print_job():
	pass

def main():
	updater = Updater(TOKEN)
	j = updater.job_queue

	j.run_once(print_job, 10, name='job1')
	j.run_once(print_job, 10, name='job2')
	j.run_once(print_job, 10, name='job3')

	try:
		print('text11:', j.select_job())
	except Exception as e:
		print('test12: ',e)

	try:
		print('text21:', j.select_job(name=None))
	except Exception as e:
		print('test22: ',e)

	try:
		print('test31:', j.select_job('job1'))
	except Exception as e:
		print('test32:', e)

	try:
		print('test41', j.select_job('job1', 'job2'))
	except Exception as e:
		print('test42', e)

	try:
		print('test51:', job_queue.select_job('job4'))
	except Exception as e:
		print('test52:', e)

	print('job_list = ', j.jobs())

	time.sleep(10)

if __name__ == '__main__':
	main()

outputs:

test12:  select_job() missing 1 required positional argument: 'name'
text21: ()
test31: (<telegram.ext.jobqueue.Job object at 0x7fd0ae7b3828>,)
test42 select_job() takes 2 positional arguments but 3 were given
test51: ()
job_list =  (<telegram.ext.jobqueue.Job object at 0x7fd0ae7b3828>, 
             <telegram.ext.jobqueue.Job object at 0x7fd0ae7b3940>,
             <telegram.ext.jobqueue.Job object at 0x7fd0ae7b3a58>)

@tsnoam At sleep duration I run the same script, multiple time by my machine and it seems thread safe. If this method is not thread safe so the method jobs() is not thread safe too, and I prefer to ignore this contribution.

@paradox70 Look in the test folder for test_jobqueue. The script you pasted is basicaly what I would like to see in there. Except for prints use assert and for try: ... except:... use pytest.raises.

Regarding the threadsafety, we will have to look into that a bit more. I also alerted @tsnoam it's practically identical to the jobs attribute.

@paradox70
you are correct, JobQueue.jobs() is using the same mechanism as well and it isn't thread safe either and must be fixed.
we need to consider this issue, but for the moment we cannot accept your PR (we'll leave it open though).

@paradox70
We've fixed the blocking issue of thread safety.
As I see it, we'll be ready to merge your PR given the following changes:

1. Rebase on top of master and implement the same locking mechanism used with JobQueue.jobs.
2. Add unitests.
3. I prefer the name get_jobs_by_name rather than select_jobs.

Due to lack of response from PR creator, closing the PR.
We'll continue the work on a different PR.