Skip to content

Reduce code duplication in testing defaults handling #3419

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.

Sign up for GitHub

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

Jump to bottom
Merged
Bibo-Joshi merged 6 commits into from
Dec 12, 2022
Merged

Reduce code duplication in testing defaults handling #3419

Bibo-Joshi merged 6 commits into from
Dec 12, 2022

Conversation

@Bibo-Joshi
Copy link
Member

@Bibo-Joshi Bibo-Joshi commented Dec 11, 2022

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.

@Bibo-Joshi Bibo-Joshi added the ⚙️ tests affected functionality: tests label Dec 11, 2022
harshil21
harshil21 reviewed Dec 11, 2022
View reviewed changes
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

@Bibo-Joshi
Copy link
Member Author

Bibo-Joshi commented Dec 11, 2022

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

harshil21 commented Dec 11, 2022

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

harshil21
harshil21 approved these changes Dec 11, 2022
View reviewed changes
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
@Bibo-Joshi Bibo-Joshi mentioned this pull request Dec 12, 2022
Closed
1 task
@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.

Reviewers

@harshil21 harshil21 harshil21 approved these changes

Assignees

No one assigned

Labels

⚙️ tests affected functionality: tests

Projects

None yet

Milestone

v20.0

Development

Successfully merging this pull request may close these issues.

[REFACTOR] Solve duplication of TestBot.test_defaults_handling and check_defaults_handling

3 participants

@Bibo-Joshi @harshil21