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.

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

Fix onAfterUpdate callback in tests #2801

Merged
merged 3 commits into from Mar 19, 2019

Conversation

Projects
None yet
3 participants
@xintongxia
Copy link
Contributor

commented Mar 15, 2019

  1. onAfterUpdate should be called in every test
  2. onAfterUpdate should work when data is empty

@xintongxia xintongxia requested a review from Pessimistress Mar 15, 2019

});

testCases[2].onAfterUpdate = _onAfterUpdate;

This comment has been minimized.

Copy link
@xintongxia

xintongxia Mar 15, 2019

Author Contributor

I am not quite sure about this, I am wondering is it necessary to call onAfterUpdate when there is no data?

This comment has been minimized.

Copy link
@Pessimistress

Pessimistress Mar 15, 2019

Contributor

There can be many test cases, you cannot only add the callback to one of them.
You can check if data is empty inside onAfterUpdate.

@coveralls

This comment has been minimized.

Copy link

commented Mar 15, 2019

Coverage Status

Coverage decreased (-0.2%) to 58.343% when pulling 2635e10 on xx/fix-test into eae5ed0 on master.

@coveralls

This comment has been minimized.

Copy link

commented Mar 15, 2019

Coverage Status

Coverage increased (+0.03%) to 58.53% when pulling 9d1e67b on xx/fix-test into eae5ed0 on master.

@@ -89,7 +89,9 @@ export function generateLayerTests({
onAfterUpdate(params);
// Default assert
if (params.layer.isComposite) {
assert(params.subLayers.length, 'Layer should have sublayers');
if (Object.keys(params.layer.props).length) {

This comment has been minimized.

Copy link
@Pessimistress

Pessimistress Mar 15, 2019

Contributor

Why not just check if props.data is empty? This cannot be reliable.

This comment has been minimized.

Copy link
@xintongxia

xintongxia Mar 16, 2019

Author Contributor

Because it is uncertain that no sublayer will be generated when there is no data.

This comment has been minimized.

Copy link
@Pessimistress

Pessimistress Mar 16, 2019

Contributor

What is the use case you're fixing for? Can you add description to the PR summary?

This comment has been minimized.

Copy link
@Pessimistress

Pessimistress Mar 16, 2019

Contributor

If you check GeoJsonLayer, it does not render any sub layers if data is empty. This test utility must work with the most common denominator. If a layer does not require data to render, you should add the additional check to its own tests.

@xintongxia xintongxia changed the title fix test util Fix onAfterUpdate callback in tests Mar 16, 2019

@xintongxia xintongxia merged commit 5aa5714 into master Mar 19, 2019

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@xintongxia xintongxia deleted the xx/fix-test branch Mar 19, 2019

ajduberstein added a commit to ajduberstein/deck.gl that referenced this pull request Apr 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.