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

Remove sleep from jobqueue tests. Solves #957 #1357

Conversation

Ambro17
Copy link
Contributor

@Ambro17 Ambro17 commented Feb 28, 2019

I added freezegun depdendency to requirements-dev to avoid the need of sleeping to test the jobqueue. This will hopefully help to make tests more deterministics, as sleeps are a bit treasonous with precise timings.

My approach was creating a frozen job queue fixture that yields the job queue itself and the frozen time to be manually advanced.
I also added a friendly-named helper function to 'time travel' and execute jobs due in the future, by manuallly ticking the frozen time and explicitly run due jobs calling job_queue.tick()

Any suggestions are welcome

P.S: If you feel like adding a dependency for this is overkill please read #957. Both threading.Event and semaphores were evaluated and discarded because either they fail to cover some cases (threading.Event ca)or they make too complex-to-read code (Semaphores).

Closes #957

@Ambro17 Ambro17 changed the title Remove sleep from jobqueue tests Remove sleep from jobqueue tests. Solves #957 Feb 28, 2019
@Eldinnie
Copy link
Member

Hi,

Thanks for you contribution. As you can see, tests fail because pre-commit is not happy. Did you install your environment as suggested in our contribution guide?. Can you fix the pre-commit errors so travis is happy? I will make a more in depth review afterwards.

Pieter

@jsmnbom
Copy link
Member

jsmnbom commented Feb 28, 2019

Turns out the issue wasn't necessarily with your code, but rather a config issue. Please merge from master, so we can actually see if pylint is satisfied :)

@Eldinnie
Copy link
Member

Whoops, thanks for finding that @jsmnbom

@Ambro17
Copy link
Contributor Author

Ambro17 commented Mar 2, 2019

Hi, i merged master and made some code-style fixes to improve pylint score. I did follow contributing guidelines but pylint is skipped while commiting. I inspected .pre-commit-config and it seems it only cares about files under telegram directory.

    hooks:
    -   id: pylint
        files: ^telegram/.*\.py$

Maybe we should modify files regex to include files under tests?

@jsmnbom
Copy link
Member

jsmnbom commented Apr 4, 2019

Now that's kinda curious that the CI would then get mad at you, we'll look into it at some point.

It looks like there's some problem on py2? https://travis-ci.org/python-telegram-bot/python-telegram-bot/jobs/500894791#L2188
Failures on other versions are unrelated afaik, but these seem related. Anything you can reproduce->fix ?

@Ambro17
Copy link
Contributor Author

Ambro17 commented Apr 6, 2019

Yeah, i'll see it when i have time. It's strange because it didn't happen locally on any of the times i ran it so it must be some weird config. I'll comment here any update

@plammens
Copy link
Contributor

Any updates on this? I'd be happy to help if I can. I feel like the need for deterministic tests is intensifying every time more tests for JobQueue and other time-related stuff are added, or the existing ones are extended.

@Ambro17
Copy link
Contributor Author

Ambro17 commented Nov 22, 2019

I'm not working on this anymore. it seems a problem of Travis rather than the code now so it should be easier to fix

@Ambro17 Ambro17 closed this Nov 22, 2019
@Ambro17 Ambro17 reopened this Nov 22, 2019
@Bibo-Joshi
Copy link
Member

I'm not working on this anymore. it seems a problem of Travis rather than the code now so it should be easier to fix

Hey. We switched to GitHub Actions by now and personally I haven't experienced any issues with test_jobqueue.

@Ambro17 I didn't really dive into the discussion of this pr and #957. Do you think we can close this or is there something we still need to change?

@Ambro17
Copy link
Contributor Author

Ambro17 commented May 1, 2020

This issue can be closed

@Ambro17 Ambro17 closed this May 1, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In test_jobqueue.py (maybe others), wait longer (but explicitly) for expected jobs to run
5 participants