Skip to content
This repository has been archived by the owner on Jun 3, 2024. It is now read-only.

No events #444

Merged
merged 12 commits into from
Jan 24, 2019
Merged

No events #444

merged 12 commits into from
Jan 24, 2019

Conversation

alexcjohnson
Copy link
Collaborator

Closes #433 - last in the series for plotly/dash#531

In addition, as in the other no-events PRs, I tried to unify the local and CI test runners, and make it so on CI all tests run, even if an earlier one fails, so you get a complete failure report - see 3f0b9df, @T4rk1n @Marc-Andre-Rivet are you comfortable with this?

@alexcjohnson
Copy link
Collaborator Author

@T4rk1n the dcc css test:

def test_user_supplied_css(self):
app = dash.Dash(assets_folder='test/assets')
app.layout = html.Div(className="test-input-css", children=[dcc.Input()])
self.startServer(app)
self.wait_for_element_by_css_selector('.test-input-css')
self.snapshot('styled input - width: 100%, border-color: hotpink')

is failing, seems to be due to plotly/dash#547 - can you take a look at it, see if it's just the test that needs updating or if there's a real issue with the new assets_folder logic? The same issue that percy caught in this PR, that the extra css is not showing up, I see it locally running this test both on master and on this branch.

@alexcjohnson
Copy link
Collaborator Author

the dcc css test is failing, seems to be due to plotly/dash#547

OK yeah, that was an easy fix, and after looking at it in a little more detail, that test was definitely locking down buggy behavior!

@@ -62,6 +65,7 @@
"exec-sh": "^0.3.0",
"jest": "^23.6.0",
"lodash": "^4.17.11",
"npm-run-all": "^4.1.5",
Copy link
Contributor

Choose a reason for hiding this comment

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

I do so love this package 🥇

@Marc-Andre-Rivet
Copy link
Contributor

Marc-Andre-Rivet commented Jan 24, 2019

  • Interval tests still contain fireEvent logic
  • Technically out of scope, but while updating the test scripts, could you change npm's lint command to do eslint src test instead of just src -- and fix the associated lint errors

@Marc-Andre-Rivet
Copy link
Contributor

Can you confirm what version of dash is used in your environment during the components py file generation? I have 0.35.3 locally and I'm getting a significant amount of diffs vs. what's in the PR. By default, this is running on Python 3.7 in my environment -- don't know if it should makes a difference.

@Marc-Andre-Rivet
Copy link
Contributor

Test run in dash-docs seems to be behaving fine. Once the ☝️ are fixed I'm fine with this PR.

@alexcjohnson
Copy link
Collaborator Author

Can you confirm what version of dash is used in your environment during the components py file generation?

Right, this needs to be built off the no-events branch of dash plotly/dash#550. Also I believe it needs to be built with py3 (which you're doing anyway) since that branch doesn't yet have @rpkyle's byteify fix from plotly/dash#545

"lint:py": "flake8 --ignore=E501,F401,F841,F811 test",
"test": "run-s -c test-unit test:py test:pyimport lint format:test lint:py",
"test:py": "python -m unittest test.test_integration",
"test:pyimport": "python -m unittest test.test_dash_import",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can combine them with python -m unittest discover

Copy link
Collaborator Author

@alexcjohnson alexcjohnson Jan 24, 2019

Choose a reason for hiding this comment

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

Ah good to know! But test_dash_import is funny, since it rewrites files (specifically, making a dash.py in the test folder) you can't run it in conjunction with any of the other tests. So if we wanted to use discover we'd have to make test_dash_import undiscoverable one way or another, and still run it separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, that test is strange, not sure we really need to test that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess someone put that error message in the code, so they wanted to see it in a test. But yeah, not the most useful test to be lugging around.

@alexcjohnson
Copy link
Collaborator Author

Interval tests still contain fireEvent logic

Ah good call, that came in with a community PR after my initial sweep through the code... fixed in 7d82256

Technically out of scope, but while updating the test scripts, could you change npm's lint command to do eslint src test instead of just src -- and fix the associated lint errors

Sure, easy enough -> 948c7dc

n_blur: 0,
n_blur_timestamp: -1,
n_clicks: 0,
n_clicks_timestamp: -1,
Copy link
Contributor

Choose a reason for hiding this comment

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

😺 This addition of props should be noted somewhere, had to read the code to see it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @T4rk1n -> added to CHANGELOG in 8b4936b

Kind of amazing that these two are the only events we had that weren't covered by an existing regular prop already.

Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet left a comment

Choose a reason for hiding this comment

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

Looks good to me. Maybe @T4rk1n will have additional insight.

Copy link
Contributor

@T4rk1n T4rk1n left a comment

Choose a reason for hiding this comment

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

💃

@alexcjohnson alexcjohnson merged commit 67cd5d7 into master Jan 24, 2019
@alexcjohnson alexcjohnson deleted the no-events branch January 24, 2019 21:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants