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

minor test fixes for ipyplotly_integration #975

Merged
merged 145 commits into from
Apr 18, 2018

Conversation

Kully
Copy link
Contributor

@Kully Kully commented Mar 15, 2018

Minor Test Fixes - Status

NB Tests that are not minor and require changing something in a fundamental way are still checked if all other minor tests in the module are fixed. A description/error message is provided just underneath the bullet point to specify the error message for the unsolved problem

Core

  • test_tools
    • test_make_subplots
      1 F w/ test_two_row_bottom_left
    • test_get_subplots
      same issue as test_make_subplots
    • test_configuration
  • test_plotly
    • test_plot
      __init__() got an unexpected keyword argument '_raise'
    • test_credentials
  • test_stream
  • test_offline
  • test_graph_objs
    • test_annotations
      'Annotations' object has no attribute 'validate'
    • test_append_trace
      same issue as 'test_make_subplots'
    • test_data
      'Data' object has no attribute 'validate'
    • test_error_bars
    • test_figure
      __init__() missing 1 required positional argument: 'class_strs_map'
    • test_frames
      AttributeError: 'Frames' object has no attribute 'to_string'
    • test_get_figure
      AttributeError: 'Figure' object has no attribute 'get_data'
    • test_graph_objs
    • test_graph_objs_tools
      ImportError: cannot import name 'graph_objs_tools'
    • test_plotly_base_classes
      from plotly.graph_objs.graph_objs import PlotlyDict, PlotlyList
    • test_strip_style
      AttributeError: 'Figure' object has no attribute 'strip_style'
    • test_to_string
      AttributeError: 'Figure' object has no attribute 'to_string'
    • test_update
      AttributeError: 'Data' object has no attribute 'update'
  • test_grid
    The 'xsrc' property is a string and must be specified as: - A string
  • test_get_figure
    TypeError: __init__() got an unexpected keyword argument '_raise'
  • test_api

Optional

  • test_tools
    • TestQuiver
    • TestFinanceCharts
    • TestAnnotatedHeatmap
    • TestTable
    • TestGantt
    • Test2D_Density
      ('ncontours', 'colorscale', 'reversescale', 'showscale') not valid in Scatter
  • test_matplotlylib
    • test_annotations
      AssertionError: ['line']['width'] = 1.5 should be 1.0 (I think the issue is just locally due to matplotlib ver ==2.2.2; our tests use matplotlib1.3.1)
    • test_axis_scales
      from plotly.tests.test_optional.test_matplotlylib.data.axis_scales import * causes the error
ValueError: 
    Invalid value of type 'builtins.bool' received for the 'tickmode' property of layout.xaxis
        Received value: False

    The 'tickmode' property is an enumeration that may be specified as:
      - One of the following enumeration values:
            ['auto', 'linear', 'array']
  • test_figure_factory
    • TestDistplot
    • TestStreamline
    • TestDendrogram
      X = np.array([[1, 2, 3, 4], [1, 1, 3, 4], [1, 2, 1, 4], [1, 2, 3, 1]]) dendro = ff.create_dendrogram(X) fails
    • TestTrisurf
    • TestScatterPlotMatrix
    • TestGantt
    • TestViolin (deprecated messages pop up)
    • TestFacetGrid
    • TestBullet
    • TestChoropleth (not installing geopandas, pyshp and shapely)

Current Issues/Test failures from April 12, 2018:

/Users/adamkulidjian/Desktop/Adam/Plotly/plotly.py/plotly/tests/test_optional/test_figure_factory/test_figure_factory.py
AttributeError: 'Scatter' object has no attribute 'pop'
  • no .to_dataframe (test_optional/test_graph_objs)
(venv3.6) adamkulidjian@Adams-Air:~/desktop/adam/plotly/plotly.py/plotly/tests/test_core/test_to$ clenosetests test_make_subplots.py
.......................................................F.
======================================================================
FAIL: test_two_row_bottom_left (plotly.tests.test_core.test_tools.test_make_subplots.TestMakeSubplots)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/adamkulidjian/Desktop/Adam/Plotly/plotly.py/plotly/tests/test_core/test_tools/test_make_subplots.py", line 166, in test_two_row_bottom_left
    self.assertEqual(fig.to_plotly_json(), expected.to_plotly_json())
AssertionError: {'dat[156 chars][0.0, 1.0]}, 'yaxis': {'anchor': 'x', 'domain': [0.0, 0.425]}}} != {'dat[156 chars][0.0, 1.0]}, 'yaxis': {'anchor': 'x', 'domain': [0.575, 1.0]}}}
  {'data': [],
   'layout': {'xaxis': {'anchor': 'y', 'domain': [0.0, 1.0]},
              'xaxis2': {'anchor': 'y2', 'domain': [0.0, 1.0]},
-             'yaxis': {'anchor': 'x', 'domain': [0.0, 0.425]},
?                                                    -------

+             'yaxis': {'anchor': 'x', 'domain': [0.575, 1.0]},
?                                                   +++++++

              'yaxis2': {'anchor': 'x2', 'domain': [0.575, 1.0]}}}
-------------------- >> begin captured stdout << ---------------------
This is the format of your plot grid:
[ (2,1) x2,y2 ]
[ (1,1) x1,y1 ]

-removing the to_dataframe to_string, strip_styles, get_data, and validate methods?

Kully and others added 30 commits February 18, 2018 04:27
Now handles the case where the dashes are scaled, have floating-point values, or were customized with `dashes=(N,M)`.
rewrite changelog for broken choropleth
(matplotlylib) Make convert_dash more robust to changes in matplotlib.
@Kully
Copy link
Contributor Author

Kully commented Apr 13, 2018

to_dataframe to_string, strip_styles, get_data, and validate methods.

Is _raise one of these methods as well?

@jonmmease
Copy link
Contributor

I don't think @chriddyp and I discussed _raise at all. I don't think we're going to be able to support the _raise=False option directly because validation is pretty baked into the new object hierarchy. Could you try to understand what the use cases are for the _raise=False path? If we understand that we can make an informed decision on whether we can simply remove it, or if there is anything we need to add to continue to support these use cases.

… consistent with favoring validation failure to user\nrelated to TODO #283 Issue\n new valid charts created from old PlotlyImageTest examples
@@ -474,7 +474,7 @@ def get_figure(file_owner_or_url, file_id=None, raw=False):

if raw:
return figure
return tools.get_valid_graph_obj(figure, obj_type='Figure')
Copy link
Contributor Author

@Kully Kully Apr 13, 2018

Choose a reason for hiding this comment

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

@chriddyp @jmmease
The purpose of this commit is to refactor get_valid_graph_obj function to not remove invalid properties of a figure when .get_figure() is called. I think this is important because:

  • it is consistent with Jon's preference to validation over coercing to default attributes as a default (which I agree with)
  • if a user does want to bypass validation errors, setting raw=True is still an option in the get_figure function
  • one step on the road to finishing Remove graph_objs dependency from tools.py #283 fwiw

What do y'all think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this. The user will either get a dict with whatever was on the server (raw=True) or a fully valid Figure.

Do you think the stripping of invalid properties is something that regular users are going to need? I'm not clear on why we end up with figures with invalid properties on the server in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wondered the same thing. In refactoring these tests I noticed that a few of the example figures I pulled from were 4 years old and contained params that are no longer part of the valid params.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be inclined to update the test data, but I'll leave it to you and @chriddyp to decide whether we need to support some kind of legacy property removal.

@@ -103,98 +103,99 @@ def test_single_plot(self):
expected = Figure(
data=Data(),
layout=Layout(
xaxis1=XAxis(
xaxis=XAxis(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these changes from xaxis1 -> xaxis in the constructors are no longer neccesary after the fix in the following commit:

Rework handling of unknown kwargs in datatype classes (a89dcb5)
@Kully

@jonmmease
Copy link
Contributor

@Kully The Key error 'type' exception raised in figure_factory/_scatterplot.py should be fixed by 4fb462f

@Kully
Copy link
Contributor Author

Kully commented Apr 16, 2018

The Key error 'type' exception raised in figure_factory/_scatterplot.py should be fixed

it is

@Kully
Copy link
Contributor Author

Kully commented Apr 17, 2018

@jmmease from plotly.graph_objs.graph_objs import PlotlyDict, PlotlyList doesn't work anymore. Is there a way to import PlotlyDict and PlotlyList?

@jonmmease
Copy link
Contributor

@Kully No, PlotlyDict and PlotlyList don't exist anymore

@Kully
Copy link
Contributor Author

Kully commented Apr 17, 2018

@Kully No, PlotlyDict and PlotlyList don't exist anymore

ok, I will remove their associated test

@jonmmease
Copy link
Contributor

@Kully @chriddyp I'm ready to start working on adding some new tests against the new functionality of graph_objs. So I'm going to go ahead and merge this branch into ipyplotly_integration so I can use some of the new testing utilities.

@jonmmease jonmmease merged commit f140fd8 into ipyplotly_integration Apr 18, 2018
@chriddyp
Copy link
Member

@chriddyp and I chatted about that briefly and I think he was fine with removing the to_dataframe to_string, strip_styles, get_data, and validate methods.

Yup, that sounds right to me 👍

@Kully
Copy link
Contributor Author

Kully commented Apr 23, 2018

Yup, that sounds right to me 👍

Does the same go for .update?

@Kully Kully mentioned this pull request Apr 23, 2018
@jonmmease
Copy link
Contributor

No, .update is still supported on datatypes. I think the currently failing tests are uses of .update on the deprecated list types.

@nicolaskruchten nicolaskruchten deleted the rebased-test-fixes branch June 19, 2020 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants