-
Notifications
You must be signed in to change notification settings - Fork 32
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
[test][fix] capsys fixture recognizes streams #795
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the wrong direction IMO. You are merely trying to compensate possible effects in single test cases.
What I asked for is to re-instate the situation where all tests are completely unaffected by whatever I have configured in my local system.
Point taken! Just copied the ocrd_logging.conf without any change to Since it concerns just a handful tests, I'll go and review them all. |
I did some refactoring regarding the logging initialization, to enable injection of logfiles instead of doing this implicitly. But unfortunately, this did not handle all 8 additional failures. Funny fact, that these tests are green when run isolated in VSCode, but not if running the complete testsuite. Especially the What should be really taken into account: I'd like to see some more conceptual movement from ocrd-stuff to be more like a library, with some notable exceptions (say, the workspace module). I'd like to think in terms of library functionalities with meaningful results or clear exception-handling. Actually there are loggers even on method level (IMHO really overkill) and no dedicated custom exception (like @kba : |
@kba Would it be okay to you if I do pytest refactorings of test/cli/test_workspace within the scope of this PR? Sure, thank you
I also sometimes stumbling about those same basenames or that some tests are in subdirectories and some are not. Maybe a flat hierarchy would make it less confusing for developers, e.g. |
Actually, I was not capable to re-implement some tests with pytest, therefore I left altogether 4 testcases within their prev test setups. Even two of them would fail if a logging configuration is present in USER_HOME, so I marked them to be skipped. I've cut the logging parts off |
@kba Renaming the test suite sounds reasonable though. I would even propose to move all test cases that inspect any sort of output to a dedicated test suites for logging and stdout/stderr. |
The logging has since been changed significantly. But having a logging configuration still interferes with the tests in two places. I'm fixing that and will also add a config option/environment variable |
@bertsky I could not completely reproduce what has been mentioned here (#770):
although I can confirm, that with the current test resources'
ocrd_utils/ocrd_logging.conf
all three testcases oftests/test_logging_conf.py
do fail when default consoleHandler'arg
is set to(sys.stdout,)
instead of(sys.stderr,)
. That's caused for the testcases go straight forcapsys.readouterr().err
which is obviously not present when the consoleHandler is set tosys.stdout
. Therefore the new distinction in the tests.Maybe with the decorator-tests there's some wired interference with the click-handling ...