search

jupyter / declarativewidgets

[RETIRED] Jupyter Declarative Widget Extension
http://jupyter.org/declarativewidgets/docs.html
Other
120 stars 38 forks source link

Fixing js unit test run #532

Closed lbustelo closed 7 years ago

lbustelo commented 7 years ago

This PR is to figure out why js unit tests are failing

lbustelo commented 7 years ago

Found out that the test are failing only due to the unit tests for urth-viz-table and urth-viz-scatter.

lbustelo commented 7 years ago

/cc @peller @aluu317

lbustelo commented 7 years ago

@peller It appears that the urth-viz-scatter test is failing because the urth-viz-render event never fires. The timeout happens in this beforeEach.

I was taking a look at urth-core-viz-chart-behavior. It seems that the only place that the urth-viz-render event is fired is here. I cannot find what would trigger that code to run. What triggers renderEnd.urth?

lbustelo commented 7 years ago

@aluu317 Both urth-viz-col tests are failing with


Error: Cannot read property 'updateSettings' of undefined
          HTMLElement._updateColumns at /elements/urth-viz-table/urth-viz-table.html:154
                 HTMLElement.handler at /elements/polymer/polymer.html:518
                    HTMLElement.fire at /elements/polymer/polymer.html:1277
         HTMLElement._contentChanged at /elements/urth-viz-col/urth-viz-col.html:49
  HTMLElement._complexObserverEffect at /elements/polymer/polymer.html:1572
          HTMLElement._effectEffects at /elements/polymer/polymer.html:1405
         HTMLElement._propertySetter at /elements/polymer/polymer.html:1389
           HTMLElement.__setProperty at /elements/polymer/polymer.html:1398
            HTMLElement._applyConfig at /elements/polymer/polymer.html:2007
      HTMLElement._afterClientsReady at /elements/polymer/polymer.html:2001```
lbustelo commented 7 years ago

@aluu317 It seems that the table fixtures that have urth-viz-column children are not loading.

lbustelo commented 7 years ago

I tried to force handsontable to 0.25.0 but still got the same error.

lbustelo commented 7 years ago

d38391b26e4cebe2af637d340f0f58123100da69 fixes urth-viz-table

lbustelo commented 7 years ago

For the urth-core-scatter I can confirm that the renderEnd event is never fired. It seems to be a problem specific to scatter. I see it get fired for others like bar.

peller commented 7 years ago

There was a similar regression novus/nvd3#1468 I reported for line that magically got fixed in 1.8.4, but apparently not for scatter...

I experimented with scatter and render end here https://jsfiddle.net/2wfy7g32/6/ and it appears that the event does not fire for scatter with nvd3>=1.8.3

lbustelo commented 7 years ago

Tried nvd3 pegged at 1.8.1 and tests passed locally. Let's see what travis says.

peller commented 7 years ago

Unfortunately, reverting to 1.8.1 will require us to reopen #192 and #417

poplav commented 7 years ago

Saw the image tag was updated, so if this goes in we should be able to open an issue to remove manually upgrading the irkernel based off what I saw in the docker stacks commit history

lbustelo commented 7 years ago

@poplav Nice... It think I will merge this... it will help start merging PRs. Put a PR to change the irkernel upgrade issue.

lbustelo commented 7 years ago

@peller So what do you recommend... Should I remove the test that is failing for urth-chart and leave the nvd3 1.8.4 or drop it to 1.8.1 and reopen those issues you mentioned above?

lbustelo commented 7 years ago

If I run the unit test with nvd3 1.8.4 then there is that issue with renderEnd mentioned above. But even if I do some setTimeout hackery to get beforeTest to do the right thing in the urth-viz-scatter test, then the selection tests fail.

I did a test of selection running 1.8.4 using the chart example notebook and it seemed to work fine. I'm tempted to leave the the 1.8.4 version and comment out the scatter tests.

@peller thoughts?

peller commented 7 years ago

I was looking for an nvd3 defect to reference on the remaining issue. Looks like it might be novus/nvd3#1720 and I don't see any progress. I'd also lean towards staying on 1.8.4 and commenting out the scatter tests. Aside from lacking the hook for the test framework, there were no tangible effects on usability, right?

lbustelo commented 7 years ago

Ok... so I left 1.8.4 and skipped the scatter tests.

@poplav if this goes green... Please do the merge

poplav commented 7 years ago

Ok, will merge https://github.com/lbustelo/declarativewidgets/pull/6 into here first