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

Reduce code duplication in testing defaults handling #3419

Merged
merged 6 commits into from Dec 12, 2022

Conversation

Bibo-Joshi
Copy link
Member

Closes #3414. I double checked that tests do fail if any of the defaults-handling logic in tg.Bot is removed.

  • Finds a solution to not having to call get_me after each assertion or even after checking get_me via mocking it.
  • Introduces a new directory tests/auxil with a file bot_methods_checks.py in it where I moved the respective functions from conftest.py to. If this finds approval, I would move all other non-fixture things from conf.py into separate files within tests/auxil.

Copy link
Member

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

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

I'm definitely for splitting up non-fixture things from conftest to this new directory. In fact, can you also extract the nested functions inside check_defaults_handling into new functions (maybe in the same file)? Would make the code in there more readable at least

tests/conftest.py Outdated Show resolved Hide resolved
tests/test_bot.py Outdated Show resolved Hide resolved
tests/auxil/bot_method_checks.py Show resolved Hide resolved
tests/test_bot.py Outdated Show resolved Hide resolved
@Bibo-Joshi
Copy link
Member Author

In fact, can you also extract the nested functions inside check_defaults_handling into new functions (maybe in the same file)? Would make the code in there more readable at least

Those functions are nested because they work with several of the variables defined within check_defaults_handling/check_shortcut_call. Extracting those methods would mean that I'd have to add several more arguments to them & set them with functools.partial. I don't see a compelling benefit of that tbh …

@harshil21
Copy link
Member

@Bibo-Joshi hmm alright then I'm okay with it as is

Copy link
Member

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

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

Great, I can verify if this works only after this is merged to master, but the tests passing here are a good indication it will.

@Bibo-Joshi Bibo-Joshi merged commit ff645c6 into master Dec 12, 2022
@Bibo-Joshi Bibo-Joshi deleted the method-check-tests branch December 12, 2022 09:51
@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2022
@harshil21 harshil21 added this to the v20.0 milestone Jan 2, 2023
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.

[REFACTOR] Solve duplication of TestBot.test_defaults_handling and check_defaults_handling
2 participants