-
-
Notifications
You must be signed in to change notification settings - Fork 268
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
Fix tests on some platforms #124
Conversation
Codecov Report
@@ Coverage Diff @@
## master #124 +/- ##
=======================================
Coverage 94.63% 94.63%
=======================================
Files 19 19
Lines 2648 2648
=======================================
Hits 2506 2506
Misses 142 142 Continue to review full report at Codecov.
|
Sourcery Code Quality Report✅ Merging this PR will increase code quality in the affected files by 0.12%.
Here are some functions in these files that still need a tune-up:
Legend and ExplanationThe emojis denote the absolute quality of the code:
The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request. Please see our documentation here for details on how these metrics are calculated. We are actively working on this report - lots more documentation and extra metrics to come! Help us improve this quality report! |
# input directory walking and processing | ||
assert os.system('trafilatura --inputdir "tests/resources/"') % 256 == 0 | ||
result = subprocess.run([trafilatura_bin, '--inputdir', RESOURCES_DIR]).returncode == 0 |
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.
oops I just realized that I removed the assert here @adbar
I initially wanted to assert on result
but I changed my mind and didn't add the assert back
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.
should be fixed in 07216a3
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.
error on Windows for the line that wasn't part of the tests...
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.
oh right, it's the same issue that I fixed in the multiprocess pool workers.. So adding PYTHONIOENCODING in the run command environment should fix the issue:
subprocess.run([...], env={"PYTHONIOENCODING": "utf-8"})
But I'll try on a Windows machine this week-end.
I want to see if it should be set globally in the app or if it's just a test issue on GitHub worker configuration.
I suspect we have to fix it globally and not just in the tests
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.
ok!
For Python 3.10: install a protobuf compiler for
pycld3
buildIt turns out that pycld3 doesn't have prebuilt wheels yet for Python 3.10, so we need the tools to build it.
I just saw that you already opened an issue for that Wheel building fails for Python nightly (3.10) bsolomon1124/pycld3#18
Using
% 256 == 0
in tests was actually masking failures: 256 return code meant that the binary wasn't foundExplicitly call
./bin/trafilatura
or./Scripts/trafilatura
so that we don't have rely on the binary being in the PATH.Also use the more modern
subprocess.run()
instead ofos.system
.We could collect
stdout
to test more things but it didn't work the same on all platforms so I didn't add it and use the return code only.A weird encoding issue happened on Windows from processes spawned by
multiprocessing.Pool
(and only in this case)I could reproduce using
export PYTHONIOENCODING='cp1252'
before running tests.Setting PYTHONIOENCODING='utf-8' during the test fixes the issue.
Maybe we should set it in the application, if someone reports encoding issues on Windows. I didn't want to fix something that wasn't broken so I just fixed the tests.