This PR is the second part of #6497 to ease the integration, following the new plan propose by @bmw here: #6497 (comment)
This PR creates the module certbot.compat.os, that delegates everything to os, and that will be the safeguard against problematic methods of the standard module. On top of that, a quality check wrapper is called in the lint tox environment. This wrapper calls pylint and ensures that standard os module is no used directly in the certbot codebase.
Finally local oldest requirements are updated to ensure that tests will take the new logic when running.
* Add executable permissions
* Add the delegate certbot.compat.os module, add check coding style to enforce usage of certbot.compat.os instead of standard os
* Load certbot.compat.os instead of os
* Move existing compat test
* Update local oldest requirements
* Import sys
* Update account_test.py
* Update os.py
* Update os.py
* Update local oldest requirements
* Implement the new linter_plugin
* Fix local oldest for nginx
* Remove check coding style
* Update linter_plugin.py
* Add several comments
* Update the setup.py
* Add documentation
* Update acme dependencies
* Update certbot/compat/os.py
* Update docs/contributing.rst
* Update linter_plugin.py
* Handle os.path. Simplify checker.
* Add a comment to a reference implementation
* Update changelog
* Fix module registering
* Update docs/contributing.rst
* Update config and changelog
So merging the study from @bmw and me, here is what happened.
Each invocation of `certbot.logger.post_arg_parse_setup` create a file handler on `letsencrypt.log`. This function also set an atexit handler invoking `logger.shutdown()`, that have the effect to close all logger file handler not already closed at this point. This method is supposed to be called when a python process is close to exit, because it makes all logger unable to write new logs on any handler.
Before #6667 and this PR, for tests, the atexit handle would be triggered only at the end of the pytest process. It means that each test that launches `certbot.logger.post_arg_parse_setup` add a new file handler. These tests were typically connecting the file handler on a `letsencrypt.log` located in a temporary directory, and this directory and content was wipped out at each test tearDown. As a consequence, the file handles, not cleared from the logger, were accumulating in the logger, with all of them connected to a deleted file log, except the last one that was just created by the current test. Considering the number of tests concerned, there were ~300 file handler at the end of pytest execution.
One can see that, on prior #6667, by calling `print(logger.getLogger().handlers` on the `tearDown` of these tests, and see the array growing at each test execution.
Even if this represent a memory leak, this situation was not really a problem on Linux: because a file can be deleted before it is closed, it was only meaning that a given invocation of `logger.debug` for instance, during the tests, was written in 300 log files. The overhead is negligeable. On Windows however, the file handlers were failing because you cannot delete a file before it is closed.
It was one of the reason for #6667, that added a call to `logging.shutdown()` at each test tearDown, with the consequence to close all file handlers. At this point, Linux is not happy anymore. Any call to `logger.warn` will generate an error for each closed file handler. As a file handler is added for each test, the number of errors grows on each test, following an arithmetical suite divergence.
On `test_sdists.py`, that is using the bare setuptools test suite without output capturing, we can see the damages. The total output takes 216000 lines, and 23000 errors are generated. A decent machine can support this load, but a not a small AWS instance, that is crashing during the execution. Even with pytest, the captured output and the memory leak become so large that segfaults are generated.
On the current PR, the problem is solved, by resetting the file handlers array on the logging system on each test tearDown. So each fileHandler is properly closed, and removed from the stack. They do not participate anymore in the logging system, and can be garbage collected. Then we stay on always one file handler opened at any time, and tests can succeed on AWS instances.
For the record, here is all the places where the logging system is called and fail if there is still file handlers closed but not cleaned (extracted from the original huge output before correction):
```
Logged from file account.py, line 116
Logged from file account.py, line 178
Logged from file client.py, line 166
Logged from file client.py, line 295
Logged from file client.py, line 415
Logged from file client.py, line 422
Logged from file client.py, line 480
Logged from file client.py, line 503
Logged from file client.py, line 540
Logged from file client.py, line 601
Logged from file client.py, line 622
Logged from file client.py, line 750
Logged from file cli.py, line 220
Logged from file cli.py, line 226
Logged from file crypto_util.py, line 101
Logged from file crypto_util.py, line 127
Logged from file crypto_util.py, line 147
Logged from file crypto_util.py, line 261
Logged from file crypto_util.py, line 283
Logged from file crypto_util.py, line 307
Logged from file crypto_util.py, line 336
Logged from file disco.py, line 116
Logged from file disco.py, line 124
Logged from file disco.py, line 134
Logged from file disco.py, line 138
Logged from file disco.py, line 141
Logged from file dns_common_lexicon.py, line 45
Logged from file dns_common_lexicon.py, line 61
Logged from file dns_common_lexicon.py, line 67
Logged from file dns_common.py, line 316
Logged from file dns_common.py, line 64
Logged from file eff.py, line 60
Logged from file eff.py, line 73
Logged from file error_handler.py, line 105
Logged from file error_handler.py, line 110
Logged from file error_handler.py, line 87
Logged from file hooks.py, line 248
Logged from file main.py, line 1071
Logged from file main.py, line 1075
Logged from file main.py, line 1189
Logged from file ops.py, line 122
Logged from file ops.py, line 325
Logged from file ops.py, line 338
Logged from file reporter.py, line 55
Logged from file selection.py, line 110
Logged from file selection.py, line 118
Logged from file selection.py, line 123
Logged from file selection.py, line 176
Logged from file selection.py, line 231
Logged from file selection.py, line 310
Logged from file selection.py, line 66
Logged from file standalone.py, line 101
Logged from file standalone.py, line 88
Logged from file standalone.py, line 97
Logged from file standalone.py, line 98
Logged from file storage.py, line 52
Logged from file storage.py, line 59
Logged from file storage.py, line 75
Logged from file util.py, line 56
Logged from file webroot.py, line 165
Logged from file webroot.py, line 186
Logged from file webroot.py, line 187
Logged from file webroot.py, line 204
Logged from file webroot.py, line 223
Logged from file webroot.py, line 234
Logged from file webroot.py, line 235
Logged from file webroot.py, line 237
Logged from file webroot.py, line 91
```
* Reapply #6667
* Make setuptools delegates tests execution to pytest, like in acme module.
* Clean handlers at each tearDown to avoid memory leaks.
* Update changelog
The method `lock_and_call`, in `certbot.tests.util` is designed to acquire a lock on a foreign process, then execute a callable in the current process. This is done to closely reproduce the lock mechanism involved between two certbot instances that are running in parallel.
This method uses the `multiprocessing` module. But its implementation in `lock_and_call` is broken for Windows: the two processes fail to communicate, leading to a deadlock.
In fact, `multiprocessing` module is using the fork mechanism on Linux, and the spawn mechanism on Windows, leading to behavior inconsistencies between the two platforms.
As this method is for tests, and not for production code, I did not try to make two implementations "by the book", one suitable for Windows, the other for Linux, like for the `certbot.lock` module.
Instead, I use a `subprocess` approach with a trigger file allowing to coordinate the current process and the subprocess. With this, `lock_and_call` is running from the same code both on Linux and Windows.
Relevant tests in the `certbot.tests.lock_test` test module are now enabled for Windows.
* Implement new lock_and_call method
* Reactivate tests for Windows
When certbot is executing, several resources are opened. It is typically file handles and locks on them. Of course, theses resources need to be cleanup. It is done in Certbot by registering cleanup functions through atexit module, that ensures theses functions will be called when Certbot exit. This allow to not care about resource cleanup everywhere in the code, as it is processed globally.
The problem with atexit is it cleanup functions are called when the Python program exit. If the program is Certbot itself when used, this is Pytest in unit test execution. So during a unit test execution, cleanup is not called after a test and before its tearDown, but when Pytest exit, so way after tests and their respective tearDown.
But many tearDown implies to delete folders where this kind of resources are hold.
This is never a problem on Linux, thanks to its non-blocking file handling. It is usually not a problem on Windows, despite its blocking approach. But if the tearDown requires folder cleanup, exceptions are raised, and currently hidden as warnings. There is currently 504 exceptions of this type in Certbot core tests on Windows.
This PR starts to correct this situation. To do so, some of the functions cleanup normally called through atexit, are explicitly called as part of the tearDown process of relevant test classes, before directory removal is done. Theses situations come all from the certbot.tests.util.TempDirTestCase, so the code is in this specific tearDown process.
As a consequence, exceptions drop from 504 to 64.
Then there are still a significant part of them, that will be handled in later mitigation.
* Call atexit handlers before test tearDown to reduce errors on Windows
* Clear locks dict after global releasing execution
* Remove last tearDown errors.
* Clean out mock on open.
* Remove a test
* Reenable some tests
Certbot relies heavily on bash scripts to deploy a development environment and to execute tests. This is fine for Linux systems, including Travis, but problematic for Windows machines.
This PR converts all theses scripts into Python, to make them platform independant.
As a consequence, tox-win.ini is not needed anymore, and tox can be run indifferently on Windows or on Linux using a common tox.ini. AppVeyor is updated accordingly to execute tests for acme, certbot and all dns plugins. Other tests are not executed as they are for Docker, unsupported Apache/Nginx/Postfix plugins (for now) or not relevant for Windows (explicit Linux distribution tests or pylint).
Another PR will be done on certbot website to update how a dev environment can be set up.
* Replace several shell scripts by python equivalent.
* Correction on tox coverage
* Extend usage of new python scripts
* Various corrections
* Replace venv construction bash scripts by python equivalents
* Update tox.ini
* Unicode lines to compare files
* Put modifications on letsencrypt-auto-source instead of generated scripts
* Add executable permissions for Linux.
* Merge tox win tests into main tox
* Skip lock_test on Windows
* Correct appveyor config
* Update appveyor.yml
* Explicit coverage py27 or py37
* Avoid to cover non supported certbot plugins on Windows
* Update tox.ini
* Remove specific warnings during CI
* No cover on a debug code for tests only.
* Update documentation and help script on venv/venv3.py
* Customize help message for Windows
* Quote correctly executable path with potential spaces in it.
* Copy pipstrap from upstream
So here we are: after #6361 has been merged, time is to provide an environment to execute the automated testing on Windows.
Here are the assertions used to build the CI on Windows:
every test running on Linux should ultimately be runnable on Windows, in a cross-platform compatible manner (there is one or two exception, when a test does not have any meaning for Windows),
currently some tests are not runnable on Windows: theses tests are ignored by default when the environment is Windows using a custom decorator: @broken_on_windows,
test environment should have functionalities similar to Travis, in particular an execution test matrix against various versions of Python and Windows,
so test execution is done through AppVeyor, as it supports the requirements: it add a CI step along Travis and Codecov for each PR, all of this ensuring that Certbot is entirely functional on both Linux and Windows,
code in tests can be changed, but code in Certbot should be changed as little as possible, to avoid regression risks.
So far in this PR, I focused on the tests on Certbot core and ACME library. Concerning the plugins, it will be done later, for plugins which have an interest on Windows. Test are executed against Python 3.4, 3.5, 3.6 and 3.7, for Windows Server 2012 R2 and Windows Server 2016.
I succeeded at making 258/259 of acme tests to work, and 828/868 of certbot core tests to work. Most of the errors where not because of Certbot itself, but because of how the tests are written. After redesigning some test utilitaries, and things like file path handling, or CRLF/LF, a lot of the errors vanished.
I needed also to ignore a lot of IO errors typically occurring when a tearDown test process tries to delete a file before it has been closed: this kind of behavior is acceptable for Linux, but not for Windows. As a consequence, and until the tearDown process is improved, a lot of temporary files are not cleared on Windows after a test campaign.
Remaining broken tests requires a more subtile approach to solve the errors, I will correct them progressively in future PR.
Last words about tox. I did not used the existing tox.ini for now. It is just to far from what is supported on Windows: lot of bash scripts that should be rewritten completely, and that contain test logic not ready/relevant for Windows (plugin tests, Docker compilation/test, GNU distribution versatility handling and so on). So I use an independent file tox-win.ini for now, with the goal to merge it ultimately with the existing logic.
* Define a tox configuration for windows, to execute tests against Python 3.4, 3.5, 3.6 and 3.7 + code coverage on Codecov.io
* Correct windows compatibility on certbot codebase
* Correct windows compatibility on certbot display functionalities
* Correct windows compatibility on certbot plugins
* Correct test utils to run tests on windows. Add decorator to skip (permanently) or mark broken (temporarily) tests on windows
* Correct tests on certbot core to run them both on windows and linux. Mark some of them as broken on windows for now.
* Lock tests are completely skipped on windows. Planned to be replace in next PR.
* Correct tests on certbot display to run them both on windows and linux. Mark some of them as broken on windows for now.
* Correct test utils for acme on windows. Add decorator to skip (permanently) or mark broken (temporarily) tests on windows.
* Correct acme tests to run them both on windows and linux. Allow a reduction of code coverage of 1% on acme code base.
* Create AppVeyor CI for Certbot on Windows, to run the test matrix (py34,35,36,37+coverage) on Windows Server 2012 R2 and Windows Server 2016.
* Update changelog with Windows compatibility of Certbot.
* Corrections about tox, pyreadline and CI logic
* Correct english
* Some corrections for acme
* Newlines corrections
* Remove changelog
* Use os.devnull instead of /dev/null to be used on Windows
* Uid is a always a number now.
* Correct linting
* PR https://github.com/python/typeshed/pull/2136 has been merge to third-party upstream 6 months ago, so code patch can be removed.
* And so acme coverage should be 100% again.
* More compatible tests Windows+Linux
* Use stable line separator
* Remove unused import
* Do not rely on pytest in certbot tests
* Use json.dumps to another json embedding weird characters
* Change comment
* Add import
* Test rolling builds #1
* Test rolling builds #2
* Correction on json serialization
* It seems that rolling builds are not canceling jobs on PR. Revert back to fail fast code in the pipeline.
* Add and use a compatibility layer to allow certbot to be run on windows.
* Fix path comparison
* Corrections on compat and util for tests
* Less intrusive way to parse prefix in webroot plugin working for both linux and windows.
* Disable pylint import-error for some optional imports in compat.py
* Ensure path is normalized before prefixes are generated in webroot plugin
* Same prefixes in linux and windows, in fact root path is not needed in webroot plugin
* Check that user has administrative rights before continuing on windows (necessary for symlink creation)
* More straightforward way to test administrative rights on windows
* Try to resolve import error in travis ci
* OK. We go for full introspection to trick the ci.
* Move the administrative rights control to the certbot entrypoint
* Add comment for a really non trivial code.
* Allow some commands to be run on a shell without admin rights
* Avoid races conditions on windows for lock files
* Add sphinx doc to the compat functions.
* Remove irrelevant Windows error in the lock mechanism.
* Some corrections on compat
* Remove unsupported pylint disable options
* star-args removed in Pylint 1.4.3
* abstract-class-little-used removed in Pylint 1.4.3
* Fixes new lint errors
* Copy dummy-variable-rgx expression to new ignored-argument-names expression to ignore unused funtion arguments
* Notable changes
* Refactor to satisfy Pylint no-else-return warning
* Fix Pylint inconsistent-return-statements warning
* Refactor to satisfy consider-iterating-dictionary
* Remove methods with only super call to satisfy useless-super-delegation
* Refactor too-many-nested-statements where possible
* Suppress type checked errors where member is dynamically added (notably derived from josepy.JSONObjectWithFields)
* Remove None default of func parameter for ExitHandler and ErrorHandler
Resolves#5973
* update order object with returned authorizations
* major structure of finalize_order shim refactor
* util methods and imports for finalize_order shim refactor
* update certbot.tests.client_test.py
* extraneous client_test imports
* remove correct import
* update renewal call
* add test for acme.dump_pyopenssl_chain
* Add test for certbot.crypto_util.cert_and_chain_from_fullchain
* add tests for acme.client and change to fetch chain failure to TimeoutError
* s/rytpe/rtype
* remove ClientV1 passthrough
* dump the wrapped cert
* remove dead code
* remove the correct dead code
* support earlier mock
Detects acme version by checking for newNonce field in the directory, since it's mandatory. Also updates ClientNetwork.account on register and update_registration.
* add mechanism to detect acme version
* update ClientNetwork.account comment
* switch to MultiVersionClient object in acme
* add shim methods
* add returns
* use backwards-compatible format and implement register
* update to actual representation of tos v2
* add tos fields and pass through to v1 for partial updates
* update tests
* pass more tests
* allow instance variable pass-through and lint
* update certbot and tests to use new_account_and_tos method
* remove --agree-tos test from main_test for now because we moved the callback into acme
* add docstrings
* use hasattr
* all most review comments
* use terms_of_service for both v1 and v2
* add tests for acme/client.py
* tests for acme/messages.py
* Enhancement #4435. Organizing defaults in prepare_and_parse_args()
* Playing fast and loose with tox.
Discovered screwy case involving flag_default returning empty list (domains)
* Setting defaults for more low-hanging fruit. Some caveats remain.
* key_path default to None
* Applying PR feedback: explicit defaults even where redundant
* Obsessive quote consistency
* Set testing config path arguments to a 'certonly' default
* Copy the default domains list rather than get reference
* Build a testing Config from CLI_DEFAULTS
* Update some email tests for use with defaults in config.
config.email and config.noninteractive_mode in these tests
used to be magic-mock'd, so were True-ish. The default
email is now None and default noninteractive_mode is
False, so update in tests accordingly.
* Lint...
* Copy anything retrieved using flag_defaults. Apply this to test_cli_ini_domains too.
* Put those quotes back. Backslashes are just the worst.
* Remove vestigial line
* A test to ensure no regressions around modifying CLI_DEFAULTS
* Addressing #4071 Wrote an ImmutableReturnMixin to prevent developers overriding return_value in certain Mock objects
* Language
* Loosening the assumption that underlying _mock objects need to be Immutable-like simplifies implementation
* Addressing #4071
* Ensure side_effects and return_values are pushed down to the underlying _mock in FreezableMocks. And IDisplay mocks are no longer frozen in _create_get_utility_mock()
* Edit a handful of tests to not override the mock_get_utility return_value
* Brief explainer of FreezableMock.__setattr__
* Incorporating PR feedback and some compatibility
* FreezableMock __getattr__ needs a shortcut in case of return_value or side_effect
* Changing return_value only forbidden if set before freezing
* Remove unnecessary else block
* Expanded doc strings
* Bring a couple new tests in line with patch_get_utility() norms
* Addressing #4434 by implementing ConfigTestCase which mocks out a NamespaceConfig for consistent config use across tests
* Refactor account_test.py for use with ConfigTestCase
* Remove superfluous setup/teardown
* Pylint oops.
* Fix redundant inheritance class definitions
* Separate ConfigTestCase's mocked directories
* Module import style consistency
* Refactor log_test.py for use with ConfigTestCase
* Refactor eff_test.py for use with ConfigTestCase. Also tweak for import style consistency
* Refactor reverter_test.py for use with ConfigTestCase
* Refactor renewal_test.py for use with ConfigTestCase
* Refactor main_test.py for use with ConfigTestCase
* Refactor storage_test.py for use with ConfigTestCase
* Refactor cert_manager_test.py for use with ConfigTestCase
* Refactor client_test.py for use with ConfigTestCase
* Refactor configuration_test.py for use with ConfigTestCase
* Pylint!
* Incorporating PR feedback
* Remove comment
Fixes#3996.
I'm pretty confident this PR solves the problem. I've audited all calls to IDisplay methods and the assertions done in certbot.display.util are now done in all our unit tests.
With that said, it wouldn't hurt to have someone else double check I didn't miss anything. The easiest way to do this is to grep for IDisplay in our code and ensure all calls to IDisplay methods are valid. This means every method call other than notification (because a notification call is always OK) either provides a value for default or force_interactive. This is defined in interfaces.py.
I've also been considering removing the assertion that's been causing us trouble here from our release. The only argument I have for not doing so is it may hinder 3rd party plugin development. When they use IDisplay, they have the same problem as we do with prompting users without a TTY. Not keeping this assertion in makes it more likely they won't notice the issue and Certbot will crash on an unsuspecting user.
With that said, none of our known 3rd party plugins use IDisplay at all.
* Provide force_interactive in _get_certname
* Use force_interactive when asking for webroot
* Factor IDisplay assertion into it's own function
* Add util.patch_get_utility()
* Allow custom path to patch_get_utiity
* Change GetEmailTest to use patch_get_utility
* Use new_callable to create new objects
* Modify tests to use patch_get_utility
* Improve FreezableMock documentation
* Add user facing error to TTY magic
* Comment out assert_valid_call
* Add test_input_assertion_fail2()
* Begin breaking out cli_test.py
* simplify main
* refactor porse tests
* move determine account tests to main_test.py
* move duplicate cert test to main_test.py
* move cli stuff out of the way
* add test_renewal.py
* move error test into error_handler_test.py
* move test_read_file
* move test_no_gui out of MainTest
* move test_install_abspath to parsetest
* Move main tests into main_test.py
* move cli tests back into cli_test.py
* clean up cli_test.py
* move punycode test to util_test.py
* Fix NameError from missing plugins_disco
* Fix linting errors
* test_renewal.py -> renewal_test.py
* rm not_cli_test.py
* Move main._handle_exception test to main_test.py
* Move renewal import in renewal_test.py
from @ohemorange comments
* certbot.tests.test_util -> certbot.tests.util
* Fix issues from rebasing.
* Fix testing issue with option_was_set
* fix linting issue
