-
Notifications
You must be signed in to change notification settings - Fork 124
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
Include unit tests for default plugins #1684
Conversation
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.
Hi @tlylt! Good job on this PR :-) I've looked through your changes and left some comments for discussion/changes.
The tests for PlantUML looks good to me, I think the control flows for the function is well-tested. Don't think there is anything we missed out.
For the other 2 tests, I have written some minor comments.
Thanks for your contribution!
|
||
test('postProcessNode should append anchor-related HTML to heading nodes', () => { | ||
const [h1Node] = cheerio.parseHTML('<h1 id="h1-node">' | ||
+ '<span id="h1-node" class="anchor"></span>should have anchor</h1>', true); |
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.
While reviewing your PR, I went to look at some of our code for the anchor plugin and it got me thinking whether we refactor our anchor plugin. Specifically for this <span>
anchor node, we are currently adding it in processNode
file instead of adding it in our anchor plugin processNode
function.
But it seems to me like this <span>
anchor node is pretty "coupled" with the anchor plugin, since our anchor plugin wouldn't work without this <span>
anchor node. Thus, I was thinking it may be better if we shift that portion into our anchor plugin file?
It would also make more sense for this test case to include that <span>
node, in my opinion. As it stands, even if we remove this <span>
node, we are still testing the functionality of our anchor plugin function fine.
But I may be missing some context @ang-zeyu do you know anything about this? :O
@tlylt feel free to chip in as well!
|
||
test('processNode should not convert improper syntax nodes', () => { | ||
const [divSpanNode] = cheerio | ||
.parseHTML('<div><span heading>Heading</span></div>', true)[0].children; |
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.
I think it would be good to add short inline comments to explain how are these syntaxes improper, so readers don't have to refer to the function to infer what the proper syntax should be. It also serves as also a form of verification of what improper syntax we are actually testing.
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.
Hi, thanks for the comment. I have adjusted the test cases for the shorthandSyntax
plugin and included in a comment at the top to show the desired outcome. Hope that's clearer.
}, | ||
); | ||
|
||
test('processNode should not convert span node without heading attribute', () => { |
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.
I think maybe this can be included in the "improper syntax nodes" as well, but it's fine if we leave it here also. I leave it up to you :-)
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.
I decided to split the improper syntax test case into two so that it's clearer as to why it's improper. So I will keep this one separate as well. Thank you for the suggestion.
What is the purpose of this pull request?
Overview of changes:
This PR partially addresses #781. I have included unit tests for default plugins as a means to complement existing functional tests.
Anything you'd like to highlight / discuss:
In the unit test for
markbind-plugin-plantuml
, I decided to mockfs
andchild_process
modules such thatgenerateDiagram
will not actually launchplantuml.jar
to convert plantuml code into an image file. This is because that process seems to be covered in functional tests as plantuml diagrams will be generated in theexpected/diagrams
folder. I did not unit test thegenerateDiagram
function because it is not an exported function. I am not sure if that is a good reason but I am thinking since it is somewhat private and inaccessible to other modules, perhaps I should not test it.Testing instructions:
Run
npm run test
to verify that all tests are still passing. To only run the 3 added tests, cd intopackages/core/test/unit/plugins/default
and runnpx jest anchor.test.js
, ornpx jest plantuml.test.js
, ornpx jest shorthandSyntax.test.js
Proposed commit message: (wrap lines at 72 characters)
Include unit tests for default plugins
There are severe plugins that have specific functionalities
but are not unit tested.
Some of the edge cases might be harder to test with functional tests.
Let's add unit tests for default plugins to verify their functionalities.
For example,
processNode
should correctly modify thenode
passing in according to a plugin's purpose.Checklist: ☑️