Add crosschecks for objects & counts between Sphinx and sphobjinv parsing #189
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Compare sphinx.util.inventory.InventoryFile loads between (i) objects.inv files generated by Sphinx and (ii) objects.inv files imported and re-exported by sphobjinv. This is a much more fundamental round-trip test, and probably should have been in the test suite from the beginning. Many inventories currently failing; most appear to be because of the validity of names containing spaces, per #181.
Now the InventoryFile count must match the sphobjinv.Inventory count, *as well as* the InventoryFile round-trip dict must match. This might obviate some of the other tests in the suite. But, if the suite is still pretty fast, duplicative tests aren't the worst thing ever.
Still need to revise the domain/role group handling to match the new behavior to avoid a regex DOS.
Part of #182, these inventories will now never be valid, barring a change to how Sphinx handles the priority values.
Should save a BUNCH of time during debugging. [skip ci]
Celery has some duplicate objects in the inventory, which Sphinx ignores but sphobjinv does not. OpenCV has objects that are not parsed correctly by Sphinx after the regex change in sphinx-doc/sphinx#8225, but that sphobjinv currently reads without difficulty. A decision will have to be made whether to follow sphinx#8225's fix for the DOS potential reported in sphinx#8175, or implement one of the other less-susceptible regexes suggested in sphinx#8175. Prefer the latter option, as it won't require post-processing of a combined domain:role.
Should avoid nasty backtrack complexity pointed to in sphinx#8175. Change as suggested there by @yetingli
DOS fix happened between 3.2.1 and 3.3.0 [skip ci]
Changes in Sphinx behavior make some objects_xyz.inv need different success conditions when comparing counts of soi.Inventory objects and objects from sphinx.util.InventoryFile.
Codecov Report
@@ Coverage Diff @@
## master #189 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 18 18
Lines 664 664
=========================================
Hits 664 664
Continue to review full report at Codecov.
|
The test-code execution check only runs with a recent version of Sphinx, and also doesn't run with --testall. So, those cases *aren't* going to run in the AP check for whether all the test code is run.
Multiple changes to constraints on inventory object data line syntax. [skip ci]
*Believe* it's now fully up to date and accurate for all of Now need to make sure all of these are tested well enough...
Doc edits are done!
Might have been relevant if the relaxation from integer priority values had been viable, but isn't b/c it's not.
[skip ci]
Adding some hypothesis tests and DataObj validation (#152) will further strengthen confidence around (and possibly obviate) these tests.
Believe all the descriptions and constraints are correct now. Need to reread later to be sure.
Includes fix for README.rst objects count
Due to changes in Sphinx InventoryFile loading behavior at v3.3
Skip should only occur within the tox matrix when testing old dependency versions, so a # pragma: no cover for the sake of the Azure CI makes sense.
Hopefully make the mkdoc inventory pull as binary
Should be ready for merge back to `master`, and to go ahead and cut the v2.1 release.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Once finished, will close #181 and close #182 and close #183 and close #190.