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

[test][fix] capsys fixture recognizes streams #795

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

M3ssman
Copy link
Contributor

@M3ssman M3ssman commented Feb 4, 2022

@bertsky I could not completely reproduce what has been mentioned here (#770):

take the default ocrd_utils/ocrd_logging.conf and modify it in some places, say by defining a logger for qualname=PIL and level=ERROR (which will fail at
core/tests/test_decorators.py
Line 64 in 9567190 self.assertEqual(logging.getLogger('PIL').getEffectiveLevel(), logging.INFO) ), or by setting the default consoleHandler to sys.stdout instead of stderr.

although I can confirm, that with the current test resources' ocrd_utils/ocrd_logging.conf all three testcases of tests/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 for capsys.readouterr().err which is obviously not present when the consoleHandler is set to sys.stdout. Therefore the new distinction in the tests.

Maybe with the decorator-tests there's some wired interference with the click-handling ...

Copy link
Collaborator

@bertsky bertsky left a 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.

@M3ssman
Copy link
Contributor Author

M3ssman commented Feb 4, 2022

Point taken!

Just copied the ocrd_logging.conf without any change to $USER_HOME and have to learn that suddenly out of nowhere 7 tests go fail; Changed PILs Log level as you mentioned, makes the decorator module also fail. Well, looks like any test that includes validations of log output must be ensured to run in real isolated manner, far more than actually.

Since it concerns just a handful tests, I'll go and review them all.

@M3ssman
Copy link
Contributor Author

M3ssman commented Feb 5, 2022

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 test_log and the test_logging modules are tricky to decompose. Anyway IMHO this is a step in the proper direction.

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 OcrdException), which could be handled by a client application.
(Personally I'm pointing to the oai-record-loading things - I'm aware that I'm responsible for this mess, but his must be redone in a proper way. Actually it only logs a warning(!!) if the XML can't be parsed, but the response gets written anyway to disc. But that's a different story.)

@kba :
Actual 2 test cases fail from test_workspace, but this module is addressed with #779 therefore I'd suggest to first apply this one and I will afterwards review this module again before we end in a hard to manage merge situation.

@M3ssman
Copy link
Contributor Author

M3ssman commented Feb 6, 2022

Oh no, I've just learned that there are two modules named test_workspace, and the one addressed in #779 is not the one I've mentioned before.
@bertsky @kba Would it be okay to you if I do pytest refactorings of test/cli/test_workspace within the scope of this PR?

@kba
Copy link
Member

kba commented Feb 8, 2022

@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

Oh no, I've just learned that there are two modules named test_workspace,

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. tests/cli/test_workspace.py -> tests/test_cli_workspace.py`?

@M3ssman
Copy link
Contributor Author

M3ssman commented Feb 17, 2022

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 test_add_nonexisting_file and test_init_with_mets_basename_and_not_mets_deprecated . How to deal properly with the OCR-D logging issues - I still don't know. Personally, I decrease logging in my apps as far as possible an/or use a dedicated Logger-Object that encapsulates all logging-related stuff.

@M3ssman
Copy link
Contributor Author

M3ssman commented Feb 17, 2022

@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.

@kba
Copy link
Member

kba commented Nov 23, 2023

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 OCRD_LOG_BUILTIN_ONLY that globally disables picking up any ocrd_logging.conf. Then I'll cherry-pick your unitttest-to-pytest conversions and close.

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

Successfully merging this pull request may close these issues.

3 participants