-
Notifications
You must be signed in to change notification settings - Fork 487
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
Added testcases #2273
base: main
Are you sure you want to change the base?
Added testcases #2273
Conversation
Signed-off-by: Nitin Ramnani <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2273 +/- ##
=======================================
Coverage 96.59% 96.59%
=======================================
Files 254 254
Lines 7641 7641
Branches 1927 1927
=======================================
Hits 7381 7381
Misses 260 260 ☔ View full report in Codecov by Sentry. |
|
||
it('should match snapshot', () => { | ||
const wrapper = shallow(<Ticks endTime={200} numTicks={5} showLabels startTime={100} />); | ||
expect(wrapper).toMatchSnapshot() |
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.
we are trying to move away from snapshot testing. What specifically are you trying to validate?
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.
Trying to validate that if same props are passed, component should render same html. So in future if someone accidentally changes anything in the component, it will be caught by this testcase.
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.
component should render same html
but the component is not obligated to render the same HTML. Its implementation can change and the test will fail, even though the implementation can still be functionally correct. Tests should be validating use-visible behavior, e.g. you asked for 5 ticks - the result should have 5 ticks.
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.
You are correct but if we are changing implementation then we should also update the testcase. Snapshot testcase make sure until we haven't made any change in component
then HTML shouldn't also change there by help us in catching any accidentally changes.
Description of the changes
How was this change tested?
yarn test
Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test