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

Implement binary data RFC (PR 3/3) #2670

Merged
merged 1 commit into from Mar 12, 2019

Conversation

Projects
None yet
4 participants
@Pessimistress
Copy link
Contributor

commented Feb 11, 2019

For #2665

Change List

  • Add a test util autoTestLayer. It covers the following:
    • layer class should have a layerName
    • layer constructs and updates with empty props
    • layer constructs and updates with data: null
    • layer constructs and updates with array data, generates either sub layers (composite) or models (primitive)
    • layer constructs and updates with iterable data (Set)
    • layer constructs and updates with non-iterable data (new feature)
    • layer constructs and updates with new values of each prop
  • Fix an issue in LayerManager where _updateLayer errors are not propagated to setLayers. These causes testLayer to never fail.
  • Update core layer tests.

@Pessimistress Pessimistress requested review from ibgreen and jianhuang01 Feb 11, 2019

// TODO - move to @deck.gl/test-utils
import {testLayer} from '@deck.gl/test-utils';

export function autoTestLayer(t, {Layer, sampleProps = {}, assert = () => {}}) {

This comment has been minimized.

Copy link
@jianhuang01

jianhuang01 Feb 11, 2019

Contributor

Make a comment about what we can test with this new API?


testLayer({
test('ScreenGridLayer', t => {
autoTestLayer(t, {

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 4, 2019

Contributor

I would much prefer it if we could keep the testLayer and just have a function that autogenerates test cases:

testLayer({
  Layer: ScreenGridLayer,
  testCases: [
    ...autoGenerateTestCases(ScreenGridLayer),
    {
       ...

Every time I see a new function layered on instead of the documented API, I need to try to figure out what is going in the wrapping function, if it is just some thing trivial or if there is something more to it. In my opinion, by constraining the function as proposed, it become much clearer what is happening:

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 4, 2019

Contributor

Properly named, this function could perhaps even be part of test utils module?

@Pessimistress Pessimistress force-pushed the binary-support-2 branch 2 times, most recently from 1145830 to 6cf32ca Mar 8, 2019

@Pessimistress Pessimistress force-pushed the binary-support-3 branch from 0316b4b to 433dabd Mar 9, 2019

@@ -311,11 +312,13 @@ export default class LayerManager {
}

if (!oldLayer) {
this._initializeLayer(newLayer);
const err = this._initializeLayer(newLayer);

This comment has been minimized.

Copy link
@Pessimistress

Pessimistress Mar 9, 2019

Author Contributor

@ibgreen Were these errors intentionally ignored? They were blocking tests from capturing layer update errors.

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 13, 2019

Contributor

Were these errors intentionally ignored?

That would have been a long time ago... I don't have any recollection but it is certainly conceivable that errors during initialization interfered with deck.gl state keeping and that it was intentionally left out.

This code looks pretty defensive though, but I guess we'll find out if there was a reason for it now that we have made the change :)

@Pessimistress Pessimistress force-pushed the binary-support-2 branch from 6cf32ca to 32ba39c Mar 12, 2019

@Pessimistress Pessimistress force-pushed the binary-support-3 branch from 433dabd to 79b054c Mar 12, 2019

@Pessimistress Pessimistress changed the base branch from binary-support-2 to master Mar 12, 2019

@coveralls

This comment has been minimized.

Copy link

commented Mar 12, 2019

Coverage Status

Coverage increased (+1.6%) to 58.78% when pulling 79b054c on binary-support-3 into bd3c27a on master.

@Pessimistress Pessimistress merged commit 307b847 into master Mar 12, 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

@Pessimistress Pessimistress deleted the binary-support-3 branch Mar 12, 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.