Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement fit-x and fit-y #5224

Merged
merged 7 commits into from
Jul 25, 2019
Merged

Implement fit-x and fit-y #5224

merged 7 commits into from
Jul 25, 2019

Conversation

willium
Copy link
Member

@willium willium commented Jul 23, 2019 •
edited
Loading

Fixes #5133

@willium willium added WIP 🚧 and removed WIP 🚧 labels Jul 23, 2019
@willium willium requested review from kanitw and domoritz and removed request for kanitw July 24, 2019 00:24
@kanitw
Copy link
Member

kanitw commented Jul 24, 2019

Add test cases to improve test coverage? there are still quite amount of lines on compile.ts not covered.

kanitw
kanitw reviewed Jul 24, 2019
View reviewed changes
src/compile/layoutsize/parse.ts
@@ -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);
Copy link
Member

@kanitw kanitw Jul 24, 2019 •
edited
Loading

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.

@willium
Copy link
Member Author

willium commented Jul 24, 2019

Add test cases to improve test coverage? there are still quite amount of lines on compile.ts not covered.

What cases are missing re: autosize?

@kanitw
Copy link
Member

kanitw commented Jul 24, 2019

See codecov -- it's 82.53% right now.

@willium
Copy link
Member Author

willium commented Jul 24, 2019

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.

@kanitw
Copy link
Member

kanitw commented Jul 24, 2019

Somehow you have to expand compile.ts. (I don't why codecov collapses it by default).

image

I think your tests already covered these branches but I'm not sure why it doesn't look like they did. Perhaps, try to run jest with coverage locally to see if the lines are well covered?

@willium
Copy link
Member Author

willium commented Jul 24, 2019

they seem to be well covered. I definitely tested all three main cases.

@kanitw
Copy link
Member

kanitw commented Jul 24, 2019

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.)

@willium
Copy link
Member Author

willium commented Jul 25, 2019 •
edited
Loading

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 jest --coverage and it has the same 82.53%, but also running with coverage seems to break a ton of existing tests with undefined variables. 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?

kanitw
kanitw requested changes Jul 25, 2019
View reviewed changes
Copy link
Member

@kanitw kanitw left a 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', () => {
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in latest commit

@domoritz domoritz changed the base branch from 4.0 to master July 25, 2019 14:16
@domoritz
Copy link
Member

Please rebase onto master

@willium willium force-pushed the ws/fit-components branch 2 times, most recently from 207692e to 83d1435 Compare July 25, 2019 15:52
@willium
Copy link
Member Author

willium commented Jul 25, 2019

Please rebase onto master

Rebased on master... which destroyed the branch :D. Then rebased on 4.0, and we're good.

@willium willium changed the base branch from master to 4.0 July 25, 2019 16:49
@willium willium changed the base branch from 4.0 to master July 25, 2019 16:52
@willium
Copy link
Member Author

willium commented Jul 25, 2019

@kanitw, through no change at all, it seems codecov is at 96.55% now.

@kanitw kanitw merged commit 9513f1a into master Jul 25, 2019
@kanitw
Copy link
Member

kanitw commented Jul 25, 2019

🎉

@kanitw
Copy link
Member

kanitw commented Jul 25, 2019

Thanks for your contribution ;)

This was referenced Sep 14, 2020
Closed
Merged
@domoritz domoritz deleted the ws/fit-components branch May 24, 2021 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kanitw kanitw kanitw requested changes

@domoritz domoritz Awaiting requested review from domoritz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Setting only width when using autosize: fit should not use default height in compiled Vega
3 participants
@willium @kanitw @domoritz