-
Notifications
You must be signed in to change notification settings - Fork 608
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
Implement fit-x and fit-y #5224
Conversation
Add test cases to improve test coverage? there are still quite amount of lines on |
@@ -110,20 +110,12 @@ function defaultUnitSize(model: UnitModel, sizeType: 'width' | 'height'): Layout | |||
const range = scaleComponent.get('range'); | |||
|
|||
if (hasDiscreteDomain(scaleType)) { | |||
if (isVgRangeStep(range)) { | |||
const size = getViewConfigDiscreteSize(config.view, sizeType); |
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.
Looking at this I'm not sure why we should read size only from getViewConfigDiscreteSize(config.view, sizeType)
.
But given we did this previously, I don't think fixing this is within the scope of this PR.
I'm filing a separate issue (#5226), as the code here might be wrong/unnecessary.
What cases are missing re: autosize? |
See codecov -- it's 82.53% right now. |
I don't really see any code paths that are worth testing that I haven't tested. feel free to point some out, happy to write them. |
they seem to be well covered. I definitely tested all three main cases. |
I agree. but it's weird that it's not covered in the coverage reports. So I think we better investigate this before merging. (At least try to get a coverage report locally and see if they are covered. If they are not, maybe you're overlooking something subtle.) |
I'm not sure how to get it running locally... I ran |
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 also don't really want to spend a bunch of time appeasing code coverage when I know it's mistake. Unless there's something I'm missing?
Well, if locally it's still low coverage, then there is very likely a mistake in the test design that should be fixed.
Jest Runner extension in VSCode is pretty handy for debugging a specific test case. You can easily step through the test case and see if it really hits the line the coverage tool is complaining about.
In any cases, I just noticed we need minores fix for the test cases with log.wrap
.
test/compile/compile.test.ts
Outdated
}); | ||
}); | ||
|
||
it('should NOT drop fit-x in top-level properties for specified width chart', () => { |
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.
Actually, you don't need this extra level of () => {
.
See this removed code https://github.com/vega/vega-lite/pull/5224/files#diff-9885764e87ed89fc726e6fe7d9a5ceeeL127, for example.
Hopefully this isn't the cause of the low coverage, but maybe it's relevant.
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.
fixed in latest commit
4ac9dac
to
45a1372
Compare
Please rebase onto master |
207692e
to
83d1435
Compare
83d1435
to
5c3565f
Compare
Rebased on |
@kanitw, through no change at all, it seems codecov is at 96.55% now. |
🎉 |
Thanks for your contribution ;) |
Fixes #5133