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

Make tests fail on warnings; add some warnings to ignore list #1193

Merged
merged 6 commits into from
Aug 11, 2023

Conversation

artem-shelkovnikov
Copy link
Member

@artem-shelkovnikov artem-shelkovnikov commented Jul 5, 2023

This PR makes 2 things:

  1. Makes our regular tests fail if a warning happens
  2. Ignores several warnings that are not relevant to us (see comments in the ini file)

@artem-shelkovnikov artem-shelkovnikov changed the title WIP Make tests fail on warnings; add some warnings to ignore list Jul 14, 2023
@artem-shelkovnikov artem-shelkovnikov force-pushed the artem/make-tests-raise-warnings branch from e74d316 to cce14c8 Compare July 14, 2023 16:50
@timgrein timgrein mentioned this pull request Jul 20, 2023
2 tasks
@jedrazb
Copy link
Member

jedrazb commented Jul 20, 2023

👋 I digged a bit regarding the inheritance warning

  • ignore:Inheritance class RetryableAiohttpSession from ClientSession is discouraged:DeprecationWarning
    I don't see a straightforward way to fix it - the root of the warning is that the aiogoogle extends from aiohttp.ClientSession class. The inheritance looks as follows: RetryableAiohttpSession -> aiogoogle.AiohttpSession -> aiohttp.ClientSession and the warning is triggered by transitive closure of our class to aiohttp.ClientSession.

From aiohttp standpoint they prefer aggregation over inheritance, see:

That means that in order to get rid of this warning we would need to re-implement aiogoogle.AiohttpSession object with aggregation rather than inheritance from aiohttp.ClientSession - that implies copy-pasting lots of code which can become tech debt immediately when aiogoogle version gets bumped and some potentially breaking changes are introduced.

So my standpoint: let's just silence the warning 🧯

@artem-shelkovnikov artem-shelkovnikov force-pushed the artem/make-tests-raise-warnings branch from 49b7b84 to e3af0cc Compare August 10, 2023 15:39
@artem-shelkovnikov artem-shelkovnikov marked this pull request as ready for review August 11, 2023 10:18
@artem-shelkovnikov artem-shelkovnikov requested a review from a team August 11, 2023 10:18
@artem-shelkovnikov artem-shelkovnikov enabled auto-merge (squash) August 11, 2023 13:19
@artem-shelkovnikov artem-shelkovnikov merged commit 260a26f into main Aug 11, 2023
@artem-shelkovnikov artem-shelkovnikov deleted the artem/make-tests-raise-warnings branch August 11, 2023 13:25
@github-actions
Copy link

💔 Failed to create backport PR(s)

The backport operation could not be completed due to the following error:
There are no branches to backport to. Aborting.

The backport PRs will be merged automatically after passing CI.

To backport manually run:
backport --pr 1193 --autoMerge --autoMergeMethod squash

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants