No need to warn when falling back to other URL #139
Conversation
|
How did my change break all these jobs? |
|
The failures in this PR are unrelated. (There's a problem with GitHub Pages.) |
|
+1 |
|
I don't understand the test failures. Did I break something? |
No, failures are due to #140 |
pytest_mpl/plugin.py
Outdated
| u = urlopen(base_url + filename) | ||
| content = u.read() | ||
| except Exception as e: | ||
| self.logger.debug(f'Downloading {base_url + filename} failed: {repr(e)}') |
There was a problem hiding this comment.
I need to check this branch out and test this but:
What is the difference in output if we log at INFO vs DEBUG?
There was a problem hiding this comment.
INFO is probably more visible and might mess with tests that do caplog.
There was a problem hiding this comment.
Surely if info will mess with caplog then so will debug (with or without -vv)?
There was a problem hiding this comment.
I had a look at this code. By default pytest will not show any log messages for a test that passes. So currently the debug (or info) log entry is not shown unless the test also fails. However, if you run pytest with --log-cli-level debug the debug log entries will be shown for passing tests, although this also shows a huge number of matplotlib debug messages about fonts etc. The failed download message is would probably be better as an info message anyway.
How about letting the user decide if they want a warning or not? If the URL begins with ?, e.g. --mpl-baseline-path=?https://example1.com,?https://example2.com, an info message is logged, and otherwise it issues a warning so no change in current behaviour?
Co-authored-by: Stuart Mumford <[email protected]>
|
I think this is good enough, if we want to do fancier things in the future we can. |
It causes unnecessary failure when test suite is set up to turn all unhandled warnings to exceptions.
cc @dhomeier