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.

Sign up for GitHub

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

Jump to bottom

TST: Fail test suite for uncaught warnings #892

Merged
merged 13 commits into from
May 26, 2022
Merged

TST: Fail test suite for uncaught warnings #892

merged 13 commits into from
May 26, 2022

Conversation

MasterOdin
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented May 23, 2022 •
edited
Loading

Codecov Report

Merging #892 (954c8bd) into main (68515c9) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #892   +/-   ##
=======================================
  Coverage   77.90%   77.90%           
=======================================
  Files          16       16           
  Lines        4317     4317           
  Branches      813      813           
=======================================
  Hits         3363     3363           
  Misses        780      780           
  Partials      174      174

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68515c9...954c8bd. Read the comment docs.

MasterOdin added 3 commits May 23, 2022 05:29
@MasterOdin
fail suite on any uncaught warnings
e17ed0d 
Signed-off-by: Matthew Peveler <[email protected]>
@MasterOdin
ignore python 2.7 deprecation warnings
e351b99 
Signed-off-by: Matthew Peveler <[email protected]>
@MasterOdin
remove unnecessary quotes
5a06dd2 
Signed-off-by: Matthew Peveler <[email protected]>
@MasterOdin MasterOdin changed the title TST: Test for UserWarning for deprecated functions TST: Fail test suite for uncaught warnings May 23, 2022
@MasterOdin MasterOdin mentioned this pull request May 23, 2022
Closed
neutrinoceros
neutrinoceros reviewed May 23, 2022
View reviewed changes
Copy link

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for doing this, I really appreciate it ! here are a couple additional recommendations.

pytest.ini Outdated
@@ -1,4 +1,7 @@
[pytest]
filterwarnings =
error
ignore:Python 3.5 and older support will be dropped with PyPDF2 2.0.0:PendingDeprecationWarning
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that you can setup such filters dynamically so they only affect a subset of your CI jobs.
This would be done using conftest.py and defining a pytest_configure function in it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not know this, looks like we would remove the second line from the pytest.ini and then add something like the following to conftest.py at the root of the test directory:

import sys


def pytest_configure(config):
    if sys.version_info < (3,6):
        config.addinivalue_line("filterwarnings", "ignore:Python 3.5 and older support will be dropped with PyPDF2 2.0.0:PendingDeprecationWarning")

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, that's exactly what I meant !

Tests/test_utils.py Outdated Show resolved Hide resolved
@MartinThoma
Copy link
Member

pytest.PytestConfigWarning: assertions not in test modules or plugins will be ignored because assert statements are not executed by the underlying Python interpreter (are you using python -O?)

Does anybody know how to deal with that? I think I just fixed the other issues (at least when I run it locally I don't get uncaught warnings from pytest anymore)

@neutrinoceros
Copy link

I think you can ignore this specific warning. I assume you are testing with -O on purpose.

@MartinThoma
Copy link
Member

Yes, we had a bug in the past that only appeared with the - o flag (I think it was related to docstrings being removed).

@MasterOdin MasterOdin force-pushed the tst-user-warnings branch from 7b3eaa5 to f84c769 Compare May 23, 2022 14:46
@MasterOdin MasterOdin force-pushed the tst-user-warnings branch from f84c769 to 490dc94 Compare May 23, 2022 14:59
@MasterOdin
Copy link
Member Author

Removing the specific case of -OO for 3.10.1 doesn't seem like it caused a unique test failure there, but given the number of raised warnings, it's hard to fully tell within this branch. Ideally #770 would have a description that explains why this was added, but will worry about that warning later, will just ignore it for now.

@MasterOdin
Copy link
Member Author

@MartinThoma For deprecation warnings that happen within the test suite, would you prefer to just update the references to use the new function names, and also add a test for the warning, or just not worry about the deprecation warnings for testing? Don't want to cause too much unnecessary churn between the main and 2.0.0-dev branches.

@MasterOdin
Copy link
Member Author

@MartinThoma With main now pointing at the 2.0.0 code, I'm going to work to land this PR onto that branch specifically, and then can backport it (along with 1.x specific stuff) to 1.x if we want

@MartinThoma
Copy link
Member

@MasterOdin Thank you for taking care of the warnings topic 🤗

MartinThoma
MartinThoma reviewed May 26, 2022
View reviewed changes
pytest.ini Outdated
Comment on lines 4 to 6
# TODO: this is related to running test suite for 3.10.1 with -OO which filters out asserts
# Do we still need 3.10.1 specific testing?
ignore::pytest.PytestConfigWarning
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could remove it: #901

@MasterOdin MasterOdin marked this pull request as ready for review May 26, 2022 13:10
@MasterOdin
Copy link
Member Author

MasterOdin commented May 26, 2022 •
edited
Loading

Well, I guess this PR is ready to be merged as #900 fixed all remaining warnings, though I do dislike these omnibus PRs as cannot cherry-pick the commit onto 1.x, have to do it much more by hand 😬

@MartinThoma
Copy link
Member

I do dislike these omnibus PRs as cannot cherry-pick the commit onto 1.x

Sorry, I know it's a bad habit 🙈

@MartinThoma MartinThoma merged commit a51e358 into main May 26, 2022
@MartinThoma MartinThoma deleted the tst-user-warnings branch May 26, 2022 13:33
MartinThoma added a commit that referenced this pull request Jun 1, 2022
@MartinThoma
REL: 2.0.0
f261bad 
The 2.0.0 release of PyPDF2 includes three core changes:

1. Dropping support for Python 3.5 and older.
2. Introducing type annotations.
3. Interface changes, mostly to have PEP8-compliant names

We introduced a [deprecation process](#930)
that hopefully helps users to avoid unexpected breaking changes.

Breaking Changes(DEP):
- PyPDF2 2.0 requires Python 3.6+. Python 2.7 and 3.5 support were dropped.
- PdfFileReader: The "warndest" parameter was removed
- PdfFileReader and PdfFileMerger no longer have the `overwriteWarnings`
  parameter. The new behavior is `overwriteWarnings=False`.
- merger: OutlinesObject was removed without replacement.
- merger.py ➔ _merger.py: You must import PdfFileMerger from PyPDF2 directly.
- utils:
  * `ConvertFunctionsToVirtualList` was removed
  * `formatWarning` was removed
  * `isInt(obj)`: Use `instance(obj, int)` instead
  * `u_(s)`: Use `s` directly
  * `chr_(c)`: Use `chr(c)` instead
  * `barray(b)`: Use `bytearray(b)` instead
  * `isBytes(b)`: Use `instance(b, type(bytes()))` instead
  * `xrange_fn`: Use `range` instead
  * `string_type`: Use `str` instead
  * `isString(s)`: Use `instance(s, str)` instead
  * `_basestring`: Use `str` instead
  * All Exceptions are now in `PyPDF2.errors`:
    - PageSizeNotDefinedError
    - PdfReadError
    - PdfReadWarning
    - PyPdfError
- `PyPDF2.pdf` (the `pdf` module) no longer exists. The contents were moved with
  the library. You should most likely import directly from `PyPDF2` instead.
  The `RectangleObject` is in `PyPDF2.generic`.
- The `Resources`, `Scripts`, and `Tests` will no longer be part of the distribution
  files on PyPI. This should have little to no impact on most people. The
  `Tests` are renamed to `tests`, the `Resources` are renamed to `resources`.
  Both are still in the git repository. The `Scripts` are now in
  https://github.com/py-pdf/cpdf. `Sample_Code` was moved to the `docs`.

For a full list of deprecated functions, please see the changelog of version
1.28.0.

New Features (ENH):
-  Improve space setting for text extraction (#922)
-  Allow setting the decryption password in PdfReader.__init__ (#920)
-  Add Page.add_transformation (#883)

Bug Fixes (BUG):
-  Fix error adding transformation to page without /Contents (#908)

Robustness (ROB):
-  Cope with invalid length in streams (#861)

Documentation (DOC):
-  Fix style of 1.25 and 1.27 patch notes (#927)
-  Transformation (#907)

Developer Experience (DEV):
-  Create flake8 config file (#916)
-  Use relative imports (#875)

Maintenance (MAINT):
-  Use Python 3.6 language features (#849)
-  Add wrapper function for PendingDeprecationWarnings (#928)
-  Use new PEP8 compliant names (#884)
-  Explicitly represent transformation matrix (#878)
-  Inline PAGE_RANGE_HELP string (#874)
-  Remove unnecessary generics imports (#873)
-  Remove star imports (#865)
-  merger.py ➔ _merger.py (#864)
-  Type annotations for all functions/methods (#854)
-  Add initial type support with mypy (#853)

Testing (TST):
-  Regression test for xmp_metadata converter (#923)
-  Checkout submodule sample-files for benchmark
-  Add text extracting performance benchmark
-  Use new PyPDF2 API in benchmark (#902)
-  Make test suite fail for uncaught warnings (#892)
-  Remove -OO testrun from CI (#901)
-  Improve tests for convert_to_int (#899)

Full Changelog: 1.28.4...2.0.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neutrinoceros neutrinoceros neutrinoceros left review comments

@MartinThoma MartinThoma MartinThoma left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MasterOdin @MartinThoma @neutrinoceros