-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
feat_content-unrestrictedFileUpload #79
feat_content-unrestrictedFileUpload #79
Conversation
Codecov Report
@@ Coverage Diff @@
## main #79 +/- ##
=======================================
Coverage 40.31% 40.31%
=======================================
Files 11 11
Lines 191 191
Branches 41 41
=======================================
Hits 77 77
Misses 111 111
Partials 3 3 Help us with your feedback. Take ten seconds to tell us how you rate us. |
@@ -4,13 +4,13 @@ import { render, screen, waitFor } from "@testing-library/react"; | |||
import { Content } from "../Components/Content"; | |||
import testFixture from "./fixtures"; | |||
|
|||
jest.mock('../Utilities/Utils', () => { | |||
jest.mock("../Utilities/Utils", () => { |
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.
One question, @Dripcoding do we need to do this for each type of vulnerability? There are 12-15 types and may be more in future.
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.
yes we'll have to do it for each test-suite. I can work on centralizing the unit test mocks in my next PR.
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.
sure, we can move this one to central place. I have question that we are adding commandInjection.test.tsx and similarly we are adding for unrestrictedFileUpload so do we need to add for all? All this data gets generated from JSON so i think having 1-2 should be sufficient. thoughts?
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.
That is a good point. Usually one either tries to write tests covering each permutation of a component or write tests that sufficiently cover the component logic given some input. I usually like to do the former when I can.
As a middle ground I can try to create a single test suite using the different vulnerability JSON payloads as parameters.
Let me know what you think.
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.
Makes sense. please go ahead with what ever makes you more comfortable.
Adding unit tests for UnrestrictedFileUpload content