Refactor tests for RST support #34
Conversation
These aren't necessary for running the tests with `invoke tests`, `pytest`, or `python -m unittest`, since these already "see" the test_ method from the base class. Removing them simplifies extending the test cases to also cover RST.
This changes how input, settings, and expected output are specified in test cases. Internal logic within the TestGraphviz base class is removed, in favor of specifying all three of these explicitly. Input options are now given as a dict. The main goal of this change is to make it easier to extend these tests to cover RST too. IMO this also makes it easier to read the tests and understand which combination of settings and options are under test, and what the expected output is. A noteworthy side-effect of this change is that we now test the plugin's behavior in the case that no GRAPHVIZ_* settings are specificed, except those explicitly overridden in test cases.
Factors writing the input file and running Pelican out of setUp(), so that a corresponding test_rst can be added to the base class.
Using BeautifulSoup instead of regular expressions lets us assert that the expected elements are found with the right classes and attributes, without hard-coding whether e.g. `alt` is the first attribute written in the `img` tag. This will enable using these same assertions over HTML generated by ET.tostring in the RST implementation.
mshroyer
force-pushed
the
rst-refactor-tests
branch
from
2008a3a to
f7985d5
Compare
| f""" | ||
| {block_start}{f" [{options}] " if options else " "}dot | ||
| digraph {digraph_id if digraph_id else ""} {{ | ||
| md_input = f""" |
There was a problem hiding this comment.
I would prefer not defining a variable here (md_input) and using directly fid.write with the f-string as argument. But this is a stylistic issue. YMMV
|
I think I finished the review of this PR. If you can address my remarks in the code, it will be great! |
- Add copyright line - Break setUp parameters into config, settings, and expected - Remove md_input variable - Use variables to avoid repeating constants in test cases
mshroyer
left a comment
mshroyer
left a comment
There was a problem hiding this comment.
Thanks for the review! I think I've addressed the comments here, let me know if I'm missing anything.
I wasn't able to compose my fixes into separate fixup! commits that could both be appended to this PR and then auto-squashed onto the original commits to create a clean stack before merging. Instead, I've added a single commit addressing these change requests, as I wasn't sure what force-pushing amendments of the original commits to this branch would have done to our review history.
(If you'd prefer I push an amended version of the original stack, though, that wouldn't be difficult for me to do instead.)
| f""" | ||
| {block_start}{f" [{options}] " if options else " "}dot | ||
| digraph {digraph_id if digraph_id else ""} {{ | ||
| md_input = f""" |
I accidentally missed paramterizing one of the setUp() parameters as requested in the previous code review. This fixes it.
|
Let us keep the current history. I am going to merge your PR. Thanks for the meticulous work! |
|
Thanks for reviewing and merging! I've rebased #35 on top of main, and it's ready for review at your convenience. |
2 participants
Stacked on top of #33
For #32, this modifies the existing tests to make them more amenable to supporting reStructuredText in addition to Markdown. At a high level:
TestGraphviz, rather given explicitly in test cases alongside options and settings. I believe this will make the tests easier to read when RST support is added.test_mdmethod is factored out ofTestGraphviz, such that a correspondingtest_rstcan be added later.Given the changes to
test_output, I verified the new behavior by intentionally breaking some pre-existing test cases and making sure they would actually fail if incorrect.