Skip to content
This repository has been archived by the owner on Sep 14, 2020. It is now read-only.

Treat all warnings in tests as errors, reduce verbosity #312

Merged
merged 15 commits into from
Feb 20, 2020

Conversation

nolar
Copy link
Contributor

@nolar nolar commented Feb 12, 2020

What do these changes do?

Reduce verbosity of the CI/CD builds. Specifically, suppress all expected deprecation warnings, fix all unexpected resource-leaking warnings, and hide operator-under-test logging during tests.

Description

In Travis, the output of tests is now polluted with accidental logs, kubectl outputs, and a lot of warnings, especially the deprecation warnings. This makes it difficult to understand what is happening in the tests, and how reliable the code is.

With this PR, all warnings in tests are now considered as errors — more stricter requirements.

Due to this, numerous actual errors were detected with resource leakage (mostly unclosed event-loops and transports). They all are now fixed.

The expected deprecation warnings are now wrapped into context managers with specific phrase expectations (to exclude other unexpected warnings from being caught and suppressed). All unexpected deprecation warnings should be fixed on the code level (do not use the deprecated methods at least internally).

Beside that:

  • The test verbosity is decreased.
  • Operator-under-test logs are hidden.
  • kubectl output is hidden in e2e tests.
  • Only pytest's output is left.

As an example (for a green build, to make comparison simpler):

Was: https://travis-ci.org/nolar/kopf/jobs/647314734
Now: https://travis-ci.org/nolar/kopf/jobs/649395691

The end-users are not affected. This PR is for the internal codebase stability only.

As usually, see individual commit titles for a list of specific changes.

Issues/PRs

Issues: #13

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Mostly CI/CD automation, contribution experience

Checklist

  • The code addresses only the mentioned problem, and this problem only
  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt

@nolar nolar added the automation CI/CD: testing, linting, releasing automatically label Feb 12, 2020
@zincr
Copy link

zincr bot commented Feb 12, 2020

🤖 zincr found 0 problems , 1 warning

ℹ️ Large Commits
✅ Approvals
✅ Specification
✅ Dependency Licensing

Details on how to resolve are provided below


Large Commits

Checks all commits for large additions to a single file. Large commits should be reviewed more carefully for potential copyright and licensing issues

This file contains a substantial change, please review to determine if the change comes from an external source and if there are any copyright or licensing issues to be aware of

@zincr
Copy link

zincr bot commented Feb 12, 2020

🤖 zincr found 1 problem , 1 warning

❌ Approvals
ℹ️ Large Commits
✅ Specification
✅ Dependency Licensing

Details on how to resolve are provided below


Approvals

All proposed changes must be reviewed by project maintainers before they can be merged

Not enough people have approved this pull request - please ensure that 1 additional user, who have not contributed to this pull request approve the changes.

  • ✅ Approved by PR author @nolar
  • ❌ 1 additional approval needed
     

Large Commits

Checks all commits for large additions to a single file. Large commits should be reviewed more carefully for potential copyright and licensing issues

This file contains a substantial change, please review to determine if the change comes from an external source and if there are any copyright or licensing issues to be aware of

@nolar nolar requested review from 3abdelazim and aweller February 12, 2020 18:31
haikoschol
haikoschol previously approved these changes Feb 19, 2020
nolar added 15 commits February 20, 2020 12:28
It was not actually a problem per se, but some code checkers
complained on a possible resource leakage. So, why not fix it?
The examples are used as the e2e test. With the warnings converted
to errors, they fails quite often (randomly) due to SSL sockets
not closed properly (ResourceWarnings), noticed within the executor's
threads (which are used only for the sync-handlers).

Switching to `pykube-ng` and closing the request's session explicitly
(instead of on garbage collection) seems to fix it. So as not having
the session0owning object module-scoped.
@nolar nolar force-pushed the no-unexpected-warnings-in-tests branch from 941fa3d to 379dbb9 Compare February 20, 2020 11:28
@nolar nolar merged commit 0c82423 into zalando-incubator:master Feb 20, 2020
@nolar nolar deleted the no-unexpected-warnings-in-tests branch February 20, 2020 12:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automation CI/CD: testing, linting, releasing automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants