Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Unit tests with Jest + Enzyme and updates to Tabs component - #315

Merged
valentijnnieman merged 20 commits into
masterfrom
unit_tests_with_jest
Oct 17, 2018
Merged

Unit tests with Jest + Enzyme and updates to Tabs component#315
valentijnnieman merged 20 commits into
masterfrom
unit_tests_with_jest

Conversation

@valentijnnieman

@valentijnniemanvalentijnnieman commented Sep 26, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

This is a work-in-progress PR, rebased of off #299 - that needs to be merged first.
I'm comparing this branch to the update_scripts branch, so the commits are easier to reason about.

This adds support for Jest, a JS test runner, and Enzyme, testing tools for React. It allows us to write unit tests in JS for the components, hopefully lightening the load/dependency on our Selenium integration tests, and allowing for a more test-driven approach by running Jest in watch mode or as a VS-Code/Atom plugin.

I've written some basic unit tests for the Tabs component, and caught a couple of bugs that I've also fixed.

@plotly/dash

valentijnnieman
valentijnnieman commented Sep 26, 2018
View reviewed changes
Comment thread.babelrc
"plugins": ["transform-object-rest-spread", "styled-jsx/babel"]
},
"test": {
"plugins": ["transform-object-rest-spread", "styled-jsx/babel-test"]

@valentijnniemanvalentijnniemanSep 26, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Here we use (a brand new) plugin babel-test, so that Jest will use Babel and styled-jsx, but won't parse the classes to jsx-1324 or whatever, so that we can make assertions on them!

valentijnnieman
valentijnnieman commented Sep 26, 2018
View reviewed changes
Comment thread.circleci/config.yml
command: |
. venv/bin/activate
python --version
npm run test-unit

@valentijnniemanvalentijnniemanSep 26, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This is the only real difference apart from formatting - running the new unit tests in CircleCI.

valentijnnieman
valentijnnieman commented Sep 26, 2018
View reviewed changes
Comment threadsrc/components/Tabs.react.js Outdated
this.NoChildrenError = {
name: 'NoChildrenError',
message: 'Tabs did not have any children Tab components!',
};

@valentijnniemanvalentijnniemanSep 26, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Not really sure about this - if you have suggestions on how to do error handling here in a better way, please let me know!

@chriddyp

chriddyp commented Sep 26, 2018

Copy link
Copy Markdown
Member

I've written some basic unit tests for the Tabs component, and caught a couple of bugs that I've also fixed.

Just curious, what bugs did you find that weren't covered in the integration tests?

@rmarren1

rmarren1 commented Sep 26, 2018

Copy link
Copy Markdown
Contributor

I was getting an exception when the Tabs children is empty. Does this test that case?

@valentijnnieman

valentijnnieman commented Sep 26, 2018•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

I've written some basic unit tests for the Tabs component, and caught a couple of bugs that I've also fixed.

Just curious, what bugs did you find that weren't covered in the integration tests?

screen shot 2018-09-26 at 4 02 12 pm
This happens when you only have one Tab component as children
screen shot 2018-09-26 at 4 02 39 pm
I haven't tested if these are actual bugs that happen in Dash though. But at minimum these unit tests make the JS code more stable and less prone to breaking.

@T4rk1n

T4rk1n commented Sep 26, 2018

Copy link
Copy Markdown
Contributor

I think I broke dash 0.28.0 😞

Traceback (most recent call last):
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 2292, in wsgi_app
response = self.full_dispatch_request()
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1815, in full_dispatch_request
rv = self.handle_user_exception(e)
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1718, in handle_user_exception
reraise(exc_type, exc_value, tb)
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1813, in full_dispatch_request
rv = self.dispatch_request()
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1799, in dispatch_request
return self.view_functions[rule.endpoint](**req.view_args)
File "/home/circleci/project/venv/lib/python2.7/site-packages/dash/dash.py", line 995, in _serve_default_favicon
return flask.Response(pkgutil.get_data('dash', 'favicon.ico'),
File "/usr/local/lib/python2.7/pkgutil.py", line 589, in get_data
return loader.get_data(resource_name)
File "/usr/local/lib/python2.7/pkgutil.py", line 252, in get_data
with open(pathname, "rb") as file:
IOError: [Errno 2] No such file or directory: '/home/circleci/project/venv/lib/python2.7/site-packages/dash/favicon.ico'

@valentijnnieman

valentijnnieman commented Sep 26, 2018

Copy link
Copy Markdown
ContributorAuthor

@rmarren1 Actually no it doesn't. I was thinking about it, and thought that the Tabs component should throw an error if there are no children. What would it render? Thinking about it now, perhaps the Tabs component should not throw an exception when there are no children, in the cases where you set the children later through a callback...

@rmarren1

rmarren1 commented Sep 26, 2018

Copy link
Copy Markdown
Contributor

I think either would be acceptable, but let's use .isRequired consistently with this choice so that users will see a nicer error via the validation PR.

@valentijnnieman

valentijnnieman commented Sep 26, 2018

Copy link
Copy Markdown
ContributorAuthor

@rmarren1 Ah you're right, thanks! I'll update that.

@valentijnnieman

valentijnnieman commented Sep 26, 2018

Copy link
Copy Markdown
ContributorAuthor

The Selenium tests are failing here because of this issue: plotly/dash-renderer#84

@valentijnnieman

valentijnnieman commented Sep 26, 2018

Copy link
Copy Markdown
ContributorAuthor

@rmarren1 So I can remove these errors I'm throwing in favour of .isRequired and the upcoming validations?

@rmarren1

rmarren1 commented Sep 26, 2018

Copy link
Copy Markdown
Contributor

If you have .isRequired won't prop-types throw a nice exception similar to NoChildException anyway?

@valentijnnieman

valentijnnieman commented Oct 15, 2018

Copy link
Copy Markdown
ContributorAuthor

@rmarren1 Yeah that's what I expected, but if I set it to .isRequired it will always throw an error, even if there are children. I think there might be an issue where Dash doesn't immediately pass the children on to the dash-renderer instance of components, or something else is going wrong in that pipeline. In the new unit tests here it's running as expected, by the way.

@valentijnnieman
valentijnnieman changed the base branch from update_scripts to masterOctober 15, 2018 15:26
@valentijnniemanvalentijnnieman changed the title [WIP] Unit tests with Jest + EnzymeUnit tests with Jest + Enzyme and updates to Tabs componentOct 15, 2018
@valentijnniemanvalentijnnieman mentioned this pull request Oct 17, 2018
Closed
valentijnnieman
valentijnnieman commented Oct 17, 2018
View reviewed changes
Comment threadrequirements-dev.txt
tox-pyenv
six
plotly>=2.0.8
urllib3<=1.22

@valentijnniemanvalentijnniemanOct 17, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This caused the Python 2.7 tests to fail, got this fix from #333

valentijnnieman
valentijnnieman commented Oct 17, 2018
View reviewed changes
Comment threadsrc/components/Tab.react.js
* The tab's label
*/
label: PropTypes.children,
label: PropTypes.string,

@valentijnniemanvalentijnniemanOct 17, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Support for components/something else than a string for label will have to come later, anything else than a string doesn't really work now anyways, so best to set it to string for now.

chriddyp
chriddyp approved these changes Oct 17, 2018
View reviewed changes

@chriddypchriddyp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's also bump the versions and ship a release as soon as this is merged

@valentijnnieman
valentijnnieman merged commit 0c7ffb4 into masterOct 17, 2018
@valentijnnieman
valentijnnieman deleted the unit_tests_with_jest branch October 17, 2018 20:15
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Marc-Andre-RivetMarc-Andre-RivetAwaiting requested review from Marc-Andre-Rivet
1 more reviewer
@chriddypchriddypchriddyp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@valentijnnieman@chriddyp@rmarren1@T4rk1n
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Unit tests with Jest + Enzyme and updates to Tabs component by valentijnnieman · Pull Request #315 · plotly/dash-core-components · GitHub
Skip to content

This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Unit tests with Jest + Enzyme and updates to Tabs component - #315
Merged
valentijnnieman merged 20 commits into
masterfrom 
unit_tests_with_jest
Oct 17, 2018
Merged
Unit tests with Jest + Enzyme and updates to Tabs component#315
valentijnnieman merged 20 commits into
masterfrom 
unit_tests_with_jest
Conversation
@valentijnnieman
@valentijnniemanvalentijnnieman
commented
Sep 26, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
This is a work-in-progress PR, rebased of off #299 - that needs to be merged first.

I'm comparing this branch to the update_scripts branch, so the commits are easier to reason about.
This adds support for Jest, a JS test runner, and Enzyme, testing tools for React. It allows us to write unit tests in JS for the components, hopefully lightening the load/dependency on our Selenium integration tests, and allowing for a more test-driven approach by running Jest in watch mode or as a VS-Code/Atom plugin.
I've written some basic unit tests for the Tabs component, and caught a couple of bugs that I've also fixed.
@plotly/dash
valentijnnieman
valentijnnieman
commented
Sep 26, 2018
View reviewed changes
Comment thread.babelrc
 "plugins": ["transform-object-rest-spread", "styled-jsx/babel"]
 },
 "test": {
 "plugins": ["transform-object-rest-spread", "styled-jsx/babel-test"]
@valentijnniemanvalentijnniemanSep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Here we use (a brand new) plugin babel-test, so that Jest will use Babel and styled-jsx, but won't parse the classes to jsx-1324 or whatever, so that we can make assertions on them!
valentijnnieman
valentijnnieman
commented
Sep 26, 2018
View reviewed changes
Comment thread.circleci/config.yml
 command: |
 . venv/bin/activate
 python --version
 npm run test-unit
@valentijnniemanvalentijnniemanSep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This is the only real difference apart from formatting - running the new unit tests in CircleCI.
valentijnnieman
valentijnnieman
commented
Sep 26, 2018
View reviewed changes
Comment threadsrc/components/Tabs.react.js
Outdated

 this.NoChildrenError = {
 name: 'NoChildrenError',
 message: 'Tabs did not have any children Tab components!',
 };
@valentijnniemanvalentijnniemanSep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Not really sure about this - if you have suggestions on how to do error handling here in a better way, please let me know!
@chriddyp
chriddyp
commented
Sep 26, 2018

Copy link


Copy Markdown

Member
I've written some basic unit tests for the Tabs component, and caught a couple of bugs that I've also fixed.
Just curious, what bugs did you find that weren't covered in the integration tests?
@rmarren1
rmarren1
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
I was getting an exception when the Tabs children is empty. Does this test that case?
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I've written some basic unit tests for the Tabs component, and caught a couple of bugs that I've also fixed.
Just curious, what bugs did you find that weren't covered in the integration tests?
screen shot 2018-09-26 at 4 02 12 pm

This happens when you only have one Tab component as children
screen shot 2018-09-26 at 4 02 39 pm

I haven't tested if these are actual bugs that happen in Dash though. But at minimum these unit tests make the JS code more stable and less prone to breaking.
@T4rk1n
T4rk1n
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
I think I broke dash 0.28.0 😞
Traceback (most recent call last):
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 2292, in wsgi_app
response = self.full_dispatch_request()
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1815, in full_dispatch_request
rv = self.handle_user_exception(e)
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1718, in handle_user_exception
reraise(exc_type, exc_value, tb)
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1813, in full_dispatch_request
rv = self.dispatch_request()
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1799, in dispatch_request
return self.view_functions[rule.endpoint](**req.view_args)
File "/home/circleci/project/venv/lib/python2.7/site-packages/dash/dash.py", line 995, in _serve_default_favicon
return flask.Response(pkgutil.get_data('dash', 'favicon.ico'),
File "/usr/local/lib/python2.7/pkgutil.py", line 589, in get_data
return loader.get_data(resource_name)
File "/usr/local/lib/python2.7/pkgutil.py", line 252, in get_data
with open(pathname, "rb") as file:
IOError: [Errno 2] No such file or directory: '/home/circleci/project/venv/lib/python2.7/site-packages/dash/favicon.ico'

@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 Actually no it doesn't. I was thinking about it, and thought that the Tabs component should throw an error if there are no children. What would it render? Thinking about it now, perhaps the Tabs component should not throw an exception when there are no children, in the cases where you set the children later through a callback...
@rmarren1
rmarren1
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
I think either would be acceptable, but let's use .isRequired consistently with this choice so that users will see a nicer error via the validation PR.
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 Ah you're right, thanks! I'll update that.
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
The Selenium tests are failing here because of this issue: plotly/dash-renderer#84
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 So I can remove these errors I'm throwing in favour of .isRequired and the upcoming validations?
@rmarren1
rmarren1
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
If you have .isRequired won't prop-types throw a nice exception similar to NoChildException anyway?
@valentijnnieman
valentijnnieman
commented
Oct 15, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 Yeah that's what I expected, but if I set it to .isRequired it will always throw an error, even if there are children. I think there might be an issue where Dash doesn't immediately pass the children on to the dash-renderer instance of components, or something else is going wrong in that pipeline. In the new unit tests here it's running as expected, by the way.
@valentijnnieman
valentijnnieman
changed the base branch from
update_scripts
to
masterOctober 15, 2018 15:26
@valentijnniemanvalentijnnieman
changed the title
[WIP] Unit tests with Jest + EnzymeUnit tests with Jest + Enzyme and updates to Tabs componentOct 15, 2018
@valentijnniemanvalentijnnieman
mentioned this pull request
Oct 17, 2018

Closed

valentijnnieman
valentijnnieman
commented
Oct 17, 2018
View reviewed changes
Comment threadrequirements-dev.txt
tox-pyenv
six
plotly>=2.0.8
urllib3<=1.22
@valentijnniemanvalentijnniemanOct 17, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This caused the Python 2.7 tests to fail, got this fix from #333
valentijnnieman
valentijnnieman
commented
Oct 17, 2018
View reviewed changes
Comment threadsrc/components/Tab.react.js
 * The tab's label
 */
 label: PropTypes.children,
 label: PropTypes.string,
@valentijnniemanvalentijnniemanOct 17, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Support for components/something else than a string for label will have to come later, anything else than a string doesn't really work now anyways, so best to set it to string for now.
chriddyp
chriddyp
approved these changes
Oct 17, 2018
View reviewed changes
@chriddypchriddyp
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Let's also bump the versions and ship a release as soon as this is merged
@valentijnnieman
valentijnnieman
merged commit 0c7ffb4
into
masterOct 17, 2018
@valentijnnieman
valentijnnieman
deleted the

unit_tests_with_jest

branch
October 17, 2018 20:15
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@Marc-Andre-RivetMarc-Andre-RivetAwaiting requested review from Marc-Andre-Rivet

1 more reviewer

@chriddypchriddypchriddyp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@valentijnnieman@chriddyp@rmarren1@T4rk1n

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Unit tests with Jest + Enzyme and updates to Tabs component by valentijnnieman · Pull Request #315 · plotly/dash-core-components · GitHub
Skip to content

This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Unit tests with Jest + Enzyme and updates to Tabs component - #315
Merged
valentijnnieman merged 20 commits into
masterfrom 
unit_tests_with_jest
Oct 17, 2018
Merged
Unit tests with Jest + Enzyme and updates to Tabs component#315
valentijnnieman merged 20 commits into
masterfrom 
unit_tests_with_jest
Conversation
@valentijnnieman
@valentijnniemanvalentijnnieman
commented
Sep 26, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
This is a work-in-progress PR, rebased of off #299 - that needs to be merged first.

I'm comparing this branch to the update_scripts branch, so the commits are easier to reason about.
This adds support for Jest, a JS test runner, and Enzyme, testing tools for React. It allows us to write unit tests in JS for the components, hopefully lightening the load/dependency on our Selenium integration tests, and allowing for a more test-driven approach by running Jest in watch mode or as a VS-Code/Atom plugin.
I've written some basic unit tests for the Tabs component, and caught a couple of bugs that I've also fixed.
@plotly/dash
valentijnnieman
valentijnnieman
commented
Sep 26, 2018
View reviewed changes
Comment thread.babelrc
 "plugins": ["transform-object-rest-spread", "styled-jsx/babel"]
 },
 "test": {
 "plugins": ["transform-object-rest-spread", "styled-jsx/babel-test"]
@valentijnniemanvalentijnniemanSep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Here we use (a brand new) plugin babel-test, so that Jest will use Babel and styled-jsx, but won't parse the classes to jsx-1324 or whatever, so that we can make assertions on them!
valentijnnieman
valentijnnieman
commented
Sep 26, 2018
View reviewed changes
Comment thread.circleci/config.yml
 command: |
 . venv/bin/activate
 python --version
 npm run test-unit
@valentijnniemanvalentijnniemanSep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This is the only real difference apart from formatting - running the new unit tests in CircleCI.
valentijnnieman
valentijnnieman
commented
Sep 26, 2018
View reviewed changes
Comment threadsrc/components/Tabs.react.js
Outdated

 this.NoChildrenError = {
 name: 'NoChildrenError',
 message: 'Tabs did not have any children Tab components!',
 };
@valentijnniemanvalentijnniemanSep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Not really sure about this - if you have suggestions on how to do error handling here in a better way, please let me know!
@chriddyp
chriddyp
commented
Sep 26, 2018

Copy link


Copy Markdown

Member
I've written some basic unit tests for the Tabs component, and caught a couple of bugs that I've also fixed.
Just curious, what bugs did you find that weren't covered in the integration tests?
@rmarren1
rmarren1
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
I was getting an exception when the Tabs children is empty. Does this test that case?
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I've written some basic unit tests for the Tabs component, and caught a couple of bugs that I've also fixed.
Just curious, what bugs did you find that weren't covered in the integration tests?
screen shot 2018-09-26 at 4 02 12 pm

This happens when you only have one Tab component as children
screen shot 2018-09-26 at 4 02 39 pm

I haven't tested if these are actual bugs that happen in Dash though. But at minimum these unit tests make the JS code more stable and less prone to breaking.
@T4rk1n
T4rk1n
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
I think I broke dash 0.28.0 😞
Traceback (most recent call last):
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 2292, in wsgi_app
response = self.full_dispatch_request()
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1815, in full_dispatch_request
rv = self.handle_user_exception(e)
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1718, in handle_user_exception
reraise(exc_type, exc_value, tb)
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1813, in full_dispatch_request
rv = self.dispatch_request()
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1799, in dispatch_request
return self.view_functions[rule.endpoint](**req.view_args)
File "/home/circleci/project/venv/lib/python2.7/site-packages/dash/dash.py", line 995, in _serve_default_favicon
return flask.Response(pkgutil.get_data('dash', 'favicon.ico'),
File "/usr/local/lib/python2.7/pkgutil.py", line 589, in get_data
return loader.get_data(resource_name)
File "/usr/local/lib/python2.7/pkgutil.py", line 252, in get_data
with open(pathname, "rb") as file:
IOError: [Errno 2] No such file or directory: '/home/circleci/project/venv/lib/python2.7/site-packages/dash/favicon.ico'

@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 Actually no it doesn't. I was thinking about it, and thought that the Tabs component should throw an error if there are no children. What would it render? Thinking about it now, perhaps the Tabs component should not throw an exception when there are no children, in the cases where you set the children later through a callback...
@rmarren1
rmarren1
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
I think either would be acceptable, but let's use .isRequired consistently with this choice so that users will see a nicer error via the validation PR.
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 Ah you're right, thanks! I'll update that.
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
The Selenium tests are failing here because of this issue: plotly/dash-renderer#84
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 So I can remove these errors I'm throwing in favour of .isRequired and the upcoming validations?
@rmarren1
rmarren1
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
If you have .isRequired won't prop-types throw a nice exception similar to NoChildException anyway?
@valentijnnieman
valentijnnieman
commented
Oct 15, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 Yeah that's what I expected, but if I set it to .isRequired it will always throw an error, even if there are children. I think there might be an issue where Dash doesn't immediately pass the children on to the dash-renderer instance of components, or something else is going wrong in that pipeline. In the new unit tests here it's running as expected, by the way.
@valentijnnieman
valentijnnieman
changed the base branch from
update_scripts
to
masterOctober 15, 2018 15:26
@valentijnniemanvalentijnnieman
changed the title
[WIP] Unit tests with Jest + EnzymeUnit tests with Jest + Enzyme and updates to Tabs componentOct 15, 2018
@valentijnniemanvalentijnnieman
mentioned this pull request
Oct 17, 2018

Closed

valentijnnieman
valentijnnieman
commented
Oct 17, 2018
View reviewed changes
Comment threadrequirements-dev.txt
tox-pyenv
six
plotly>=2.0.8
urllib3<=1.22
@valentijnniemanvalentijnniemanOct 17, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This caused the Python 2.7 tests to fail, got this fix from #333
valentijnnieman
valentijnnieman
commented
Oct 17, 2018
View reviewed changes
Comment threadsrc/components/Tab.react.js
 * The tab's label
 */
 label: PropTypes.children,
 label: PropTypes.string,
@valentijnniemanvalentijnniemanOct 17, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Support for components/something else than a string for label will have to come later, anything else than a string doesn't really work now anyways, so best to set it to string for now.
chriddyp
chriddyp
approved these changes
Oct 17, 2018
View reviewed changes
@chriddypchriddyp
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Let's also bump the versions and ship a release as soon as this is merged
@valentijnnieman
valentijnnieman
merged commit 0c7ffb4
into
masterOct 17, 2018
@valentijnnieman
valentijnnieman
deleted the

unit_tests_with_jest

branch
October 17, 2018 20:15
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@Marc-Andre-RivetMarc-Andre-RivetAwaiting requested review from Marc-Andre-Rivet

1 more reviewer

@chriddypchriddypchriddyp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@valentijnnieman@chriddyp@rmarren1@T4rk1n

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Unit tests with Jest + Enzyme and updates to Tabs component by valentijnnieman · Pull Request #315 · plotly/dash-core-components · GitHub
Skip to content

This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Unit tests with Jest + Enzyme and updates to Tabs component - #315
Merged
valentijnnieman merged 20 commits into
masterfrom 
unit_tests_with_jest
Oct 17, 2018
Merged
Unit tests with Jest + Enzyme and updates to Tabs component#315
valentijnnieman merged 20 commits into
masterfrom 
unit_tests_with_jest
Conversation
@valentijnnieman
@valentijnniemanvalentijnnieman
commented
Sep 26, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
This is a work-in-progress PR, rebased of off #299 - that needs to be merged first.

I'm comparing this branch to the update_scripts branch, so the commits are easier to reason about.
This adds support for Jest, a JS test runner, and Enzyme, testing tools for React. It allows us to write unit tests in JS for the components, hopefully lightening the load/dependency on our Selenium integration tests, and allowing for a more test-driven approach by running Jest in watch mode or as a VS-Code/Atom plugin.
I've written some basic unit tests for the Tabs component, and caught a couple of bugs that I've also fixed.
@plotly/dash
valentijnnieman
valentijnnieman
commented
Sep 26, 2018
View reviewed changes
Comment thread.babelrc
 "plugins": ["transform-object-rest-spread", "styled-jsx/babel"]
 },
 "test": {
 "plugins": ["transform-object-rest-spread", "styled-jsx/babel-test"]
@valentijnniemanvalentijnniemanSep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Here we use (a brand new) plugin babel-test, so that Jest will use Babel and styled-jsx, but won't parse the classes to jsx-1324 or whatever, so that we can make assertions on them!
valentijnnieman
valentijnnieman
commented
Sep 26, 2018
View reviewed changes
Comment thread.circleci/config.yml
 command: |
 . venv/bin/activate
 python --version
 npm run test-unit
@valentijnniemanvalentijnniemanSep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This is the only real difference apart from formatting - running the new unit tests in CircleCI.
valentijnnieman
valentijnnieman
commented
Sep 26, 2018
View reviewed changes
Comment threadsrc/components/Tabs.react.js
Outdated

 this.NoChildrenError = {
 name: 'NoChildrenError',
 message: 'Tabs did not have any children Tab components!',
 };
@valentijnniemanvalentijnniemanSep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Not really sure about this - if you have suggestions on how to do error handling here in a better way, please let me know!
@chriddyp
chriddyp
commented
Sep 26, 2018

Copy link


Copy Markdown

Member
I've written some basic unit tests for the Tabs component, and caught a couple of bugs that I've also fixed.
Just curious, what bugs did you find that weren't covered in the integration tests?
@rmarren1
rmarren1
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
I was getting an exception when the Tabs children is empty. Does this test that case?
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I've written some basic unit tests for the Tabs component, and caught a couple of bugs that I've also fixed.
Just curious, what bugs did you find that weren't covered in the integration tests?
screen shot 2018-09-26 at 4 02 12 pm

This happens when you only have one Tab component as children
screen shot 2018-09-26 at 4 02 39 pm

I haven't tested if these are actual bugs that happen in Dash though. But at minimum these unit tests make the JS code more stable and less prone to breaking.
@T4rk1n
T4rk1n
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
I think I broke dash 0.28.0 😞
Traceback (most recent call last):
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 2292, in wsgi_app
response = self.full_dispatch_request()
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1815, in full_dispatch_request
rv = self.handle_user_exception(e)
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1718, in handle_user_exception
reraise(exc_type, exc_value, tb)
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1813, in full_dispatch_request
rv = self.dispatch_request()
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1799, in dispatch_request
return self.view_functions[rule.endpoint](**req.view_args)
File "/home/circleci/project/venv/lib/python2.7/site-packages/dash/dash.py", line 995, in _serve_default_favicon
return flask.Response(pkgutil.get_data('dash', 'favicon.ico'),
File "/usr/local/lib/python2.7/pkgutil.py", line 589, in get_data
return loader.get_data(resource_name)
File "/usr/local/lib/python2.7/pkgutil.py", line 252, in get_data
with open(pathname, "rb") as file:
IOError: [Errno 2] No such file or directory: '/home/circleci/project/venv/lib/python2.7/site-packages/dash/favicon.ico'

@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 Actually no it doesn't. I was thinking about it, and thought that the Tabs component should throw an error if there are no children. What would it render? Thinking about it now, perhaps the Tabs component should not throw an exception when there are no children, in the cases where you set the children later through a callback...
@rmarren1
rmarren1
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
I think either would be acceptable, but let's use .isRequired consistently with this choice so that users will see a nicer error via the validation PR.
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 Ah you're right, thanks! I'll update that.
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
The Selenium tests are failing here because of this issue: plotly/dash-renderer#84
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 So I can remove these errors I'm throwing in favour of .isRequired and the upcoming validations?
@rmarren1
rmarren1
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
If you have .isRequired won't prop-types throw a nice exception similar to NoChildException anyway?
@valentijnnieman
valentijnnieman
commented
Oct 15, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 Yeah that's what I expected, but if I set it to .isRequired it will always throw an error, even if there are children. I think there might be an issue where Dash doesn't immediately pass the children on to the dash-renderer instance of components, or something else is going wrong in that pipeline. In the new unit tests here it's running as expected, by the way.
@valentijnnieman
valentijnnieman
changed the base branch from
update_scripts
to
masterOctober 15, 2018 15:26
@valentijnniemanvalentijnnieman
changed the title
[WIP] Unit tests with Jest + EnzymeUnit tests with Jest + Enzyme and updates to Tabs componentOct 15, 2018
@valentijnniemanvalentijnnieman
mentioned this pull request
Oct 17, 2018

Closed

valentijnnieman
valentijnnieman
commented
Oct 17, 2018
View reviewed changes
Comment threadrequirements-dev.txt
tox-pyenv
six
plotly>=2.0.8
urllib3<=1.22
@valentijnniemanvalentijnniemanOct 17, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This caused the Python 2.7 tests to fail, got this fix from #333
valentijnnieman
valentijnnieman
commented
Oct 17, 2018
View reviewed changes
Comment threadsrc/components/Tab.react.js
 * The tab's label
 */
 label: PropTypes.children,
 label: PropTypes.string,
@valentijnniemanvalentijnniemanOct 17, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Support for components/something else than a string for label will have to come later, anything else than a string doesn't really work now anyways, so best to set it to string for now.
chriddyp
chriddyp
approved these changes
Oct 17, 2018
View reviewed changes
@chriddypchriddyp
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Let's also bump the versions and ship a release as soon as this is merged
@valentijnnieman
valentijnnieman
merged commit 0c7ffb4
into
masterOct 17, 2018
@valentijnnieman
valentijnnieman
deleted the

unit_tests_with_jest

branch
October 17, 2018 20:15
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@Marc-Andre-RivetMarc-Andre-RivetAwaiting requested review from Marc-Andre-Rivet

1 more reviewer

@chriddypchriddypchriddyp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@valentijnnieman@chriddyp@rmarren1@T4rk1n

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
Unit tests with Jest + Enzyme and updates to Tabs component by valentijnnieman · Pull Request #315 · plotly/dash-core-components · GitHub
Skip to content

This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Unit tests with Jest + Enzyme and updates to Tabs component - #315
Merged
valentijnnieman merged 20 commits into
masterfrom 
unit_tests_with_jest
Oct 17, 2018
Merged
Unit tests with Jest + Enzyme and updates to Tabs component#315
valentijnnieman merged 20 commits into
masterfrom 
unit_tests_with_jest
Conversation
@valentijnnieman
@valentijnniemanvalentijnnieman
commented
Sep 26, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
This is a work-in-progress PR, rebased of off #299 - that needs to be merged first.

I'm comparing this branch to the update_scripts branch, so the commits are easier to reason about.
This adds support for Jest, a JS test runner, and Enzyme, testing tools for React. It allows us to write unit tests in JS for the components, hopefully lightening the load/dependency on our Selenium integration tests, and allowing for a more test-driven approach by running Jest in watch mode or as a VS-Code/Atom plugin.
I've written some basic unit tests for the Tabs component, and caught a couple of bugs that I've also fixed.
@plotly/dash
valentijnnieman
valentijnnieman
commented
Sep 26, 2018
View reviewed changes
Comment thread.babelrc
 "plugins": ["transform-object-rest-spread", "styled-jsx/babel"]
 },
 "test": {
 "plugins": ["transform-object-rest-spread", "styled-jsx/babel-test"]
@valentijnniemanvalentijnniemanSep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Here we use (a brand new) plugin babel-test, so that Jest will use Babel and styled-jsx, but won't parse the classes to jsx-1324 or whatever, so that we can make assertions on them!
valentijnnieman
valentijnnieman
commented
Sep 26, 2018
View reviewed changes
Comment thread.circleci/config.yml
 command: |
 . venv/bin/activate
 python --version
 npm run test-unit
@valentijnniemanvalentijnniemanSep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This is the only real difference apart from formatting - running the new unit tests in CircleCI.
valentijnnieman
valentijnnieman
commented
Sep 26, 2018
View reviewed changes
Comment threadsrc/components/Tabs.react.js
Outdated

 this.NoChildrenError = {
 name: 'NoChildrenError',
 message: 'Tabs did not have any children Tab components!',
 };
@valentijnniemanvalentijnniemanSep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Not really sure about this - if you have suggestions on how to do error handling here in a better way, please let me know!
@chriddyp
chriddyp
commented
Sep 26, 2018

Copy link


Copy Markdown

Member
I've written some basic unit tests for the Tabs component, and caught a couple of bugs that I've also fixed.
Just curious, what bugs did you find that weren't covered in the integration tests?
@rmarren1
rmarren1
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
I was getting an exception when the Tabs children is empty. Does this test that case?
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I've written some basic unit tests for the Tabs component, and caught a couple of bugs that I've also fixed.
Just curious, what bugs did you find that weren't covered in the integration tests?
screen shot 2018-09-26 at 4 02 12 pm

This happens when you only have one Tab component as children
screen shot 2018-09-26 at 4 02 39 pm

I haven't tested if these are actual bugs that happen in Dash though. But at minimum these unit tests make the JS code more stable and less prone to breaking.
@T4rk1n
T4rk1n
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
I think I broke dash 0.28.0 😞
Traceback (most recent call last):
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 2292, in wsgi_app
response = self.full_dispatch_request()
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1815, in full_dispatch_request
rv = self.handle_user_exception(e)
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1718, in handle_user_exception
reraise(exc_type, exc_value, tb)
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1813, in full_dispatch_request
rv = self.dispatch_request()
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1799, in dispatch_request
return self.view_functions[rule.endpoint](**req.view_args)
File "/home/circleci/project/venv/lib/python2.7/site-packages/dash/dash.py", line 995, in _serve_default_favicon
return flask.Response(pkgutil.get_data('dash', 'favicon.ico'),
File "/usr/local/lib/python2.7/pkgutil.py", line 589, in get_data
return loader.get_data(resource_name)
File "/usr/local/lib/python2.7/pkgutil.py", line 252, in get_data
with open(pathname, "rb") as file:
IOError: [Errno 2] No such file or directory: '/home/circleci/project/venv/lib/python2.7/site-packages/dash/favicon.ico'

@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 Actually no it doesn't. I was thinking about it, and thought that the Tabs component should throw an error if there are no children. What would it render? Thinking about it now, perhaps the Tabs component should not throw an exception when there are no children, in the cases where you set the children later through a callback...
@rmarren1
rmarren1
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
I think either would be acceptable, but let's use .isRequired consistently with this choice so that users will see a nicer error via the validation PR.
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 Ah you're right, thanks! I'll update that.
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
The Selenium tests are failing here because of this issue: plotly/dash-renderer#84
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 So I can remove these errors I'm throwing in favour of .isRequired and the upcoming validations?
@rmarren1
rmarren1
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
If you have .isRequired won't prop-types throw a nice exception similar to NoChildException anyway?
@valentijnnieman
valentijnnieman
commented
Oct 15, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 Yeah that's what I expected, but if I set it to .isRequired it will always throw an error, even if there are children. I think there might be an issue where Dash doesn't immediately pass the children on to the dash-renderer instance of components, or something else is going wrong in that pipeline. In the new unit tests here it's running as expected, by the way.
@valentijnnieman
valentijnnieman
changed the base branch from
update_scripts
to
masterOctober 15, 2018 15:26
@valentijnniemanvalentijnnieman
changed the title
[WIP] Unit tests with Jest + EnzymeUnit tests with Jest + Enzyme and updates to Tabs componentOct 15, 2018
@valentijnniemanvalentijnnieman
mentioned this pull request
Oct 17, 2018

Closed

valentijnnieman
valentijnnieman
commented
Oct 17, 2018
View reviewed changes
Comment threadrequirements-dev.txt
tox-pyenv
six
plotly>=2.0.8
urllib3<=1.22
@valentijnniemanvalentijnniemanOct 17, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This caused the Python 2.7 tests to fail, got this fix from #333
valentijnnieman
valentijnnieman
commented
Oct 17, 2018
View reviewed changes
Comment threadsrc/components/Tab.react.js
 * The tab's label
 */
 label: PropTypes.children,
 label: PropTypes.string,
@valentijnniemanvalentijnniemanOct 17, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Support for components/something else than a string for label will have to come later, anything else than a string doesn't really work now anyways, so best to set it to string for now.
chriddyp
chriddyp
approved these changes
Oct 17, 2018
View reviewed changes
@chriddypchriddyp
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Let's also bump the versions and ship a release as soon as this is merged
@valentijnnieman
valentijnnieman
merged commit 0c7ffb4
into
masterOct 17, 2018
@valentijnnieman
valentijnnieman
deleted the

unit_tests_with_jest

branch
October 17, 2018 20:15
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@Marc-Andre-RivetMarc-Andre-RivetAwaiting requested review from Marc-Andre-Rivet

1 more reviewer

@chriddypchriddypchriddyp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@valentijnnieman@chriddyp@rmarren1@T4rk1n

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Unit tests with Jest + Enzyme and updates to Tabs component by valentijnnieman · Pull Request #315 · plotly/dash-core-components · GitHub
Skip to content

This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Unit tests with Jest + Enzyme and updates to Tabs component - #315
Merged
valentijnnieman merged 20 commits into
masterfrom 
unit_tests_with_jest
Oct 17, 2018
Merged
Unit tests with Jest + Enzyme and updates to Tabs component#315
valentijnnieman merged 20 commits into
masterfrom 
unit_tests_with_jest
Conversation
@valentijnnieman
@valentijnniemanvalentijnnieman
commented
Sep 26, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
This is a work-in-progress PR, rebased of off #299 - that needs to be merged first.

I'm comparing this branch to the update_scripts branch, so the commits are easier to reason about.
This adds support for Jest, a JS test runner, and Enzyme, testing tools for React. It allows us to write unit tests in JS for the components, hopefully lightening the load/dependency on our Selenium integration tests, and allowing for a more test-driven approach by running Jest in watch mode or as a VS-Code/Atom plugin.
I've written some basic unit tests for the Tabs component, and caught a couple of bugs that I've also fixed.
@plotly/dash
valentijnnieman
valentijnnieman
commented
Sep 26, 2018
View reviewed changes
Comment thread.babelrc
 "plugins": ["transform-object-rest-spread", "styled-jsx/babel"]
 },
 "test": {
 "plugins": ["transform-object-rest-spread", "styled-jsx/babel-test"]
@valentijnniemanvalentijnniemanSep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Here we use (a brand new) plugin babel-test, so that Jest will use Babel and styled-jsx, but won't parse the classes to jsx-1324 or whatever, so that we can make assertions on them!
valentijnnieman
valentijnnieman
commented
Sep 26, 2018
View reviewed changes
Comment thread.circleci/config.yml
 command: |
 . venv/bin/activate
 python --version
 npm run test-unit
@valentijnniemanvalentijnniemanSep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This is the only real difference apart from formatting - running the new unit tests in CircleCI.
valentijnnieman
valentijnnieman
commented
Sep 26, 2018
View reviewed changes
Comment threadsrc/components/Tabs.react.js
Outdated

 this.NoChildrenError = {
 name: 'NoChildrenError',
 message: 'Tabs did not have any children Tab components!',
 };
@valentijnniemanvalentijnniemanSep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Not really sure about this - if you have suggestions on how to do error handling here in a better way, please let me know!
@chriddyp
chriddyp
commented
Sep 26, 2018

Copy link


Copy Markdown

Member
I've written some basic unit tests for the Tabs component, and caught a couple of bugs that I've also fixed.
Just curious, what bugs did you find that weren't covered in the integration tests?
@rmarren1
rmarren1
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
I was getting an exception when the Tabs children is empty. Does this test that case?
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I've written some basic unit tests for the Tabs component, and caught a couple of bugs that I've also fixed.
Just curious, what bugs did you find that weren't covered in the integration tests?
screen shot 2018-09-26 at 4 02 12 pm

This happens when you only have one Tab component as children
screen shot 2018-09-26 at 4 02 39 pm

I haven't tested if these are actual bugs that happen in Dash though. But at minimum these unit tests make the JS code more stable and less prone to breaking.
@T4rk1n
T4rk1n
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
I think I broke dash 0.28.0 😞
Traceback (most recent call last):
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 2292, in wsgi_app
response = self.full_dispatch_request()
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1815, in full_dispatch_request
rv = self.handle_user_exception(e)
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1718, in handle_user_exception
reraise(exc_type, exc_value, tb)
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1813, in full_dispatch_request
rv = self.dispatch_request()
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1799, in dispatch_request
return self.view_functions[rule.endpoint](**req.view_args)
File "/home/circleci/project/venv/lib/python2.7/site-packages/dash/dash.py", line 995, in _serve_default_favicon
return flask.Response(pkgutil.get_data('dash', 'favicon.ico'),
File "/usr/local/lib/python2.7/pkgutil.py", line 589, in get_data
return loader.get_data(resource_name)
File "/usr/local/lib/python2.7/pkgutil.py", line 252, in get_data
with open(pathname, "rb") as file:
IOError: [Errno 2] No such file or directory: '/home/circleci/project/venv/lib/python2.7/site-packages/dash/favicon.ico'

@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 Actually no it doesn't. I was thinking about it, and thought that the Tabs component should throw an error if there are no children. What would it render? Thinking about it now, perhaps the Tabs component should not throw an exception when there are no children, in the cases where you set the children later through a callback...
@rmarren1
rmarren1
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
I think either would be acceptable, but let's use .isRequired consistently with this choice so that users will see a nicer error via the validation PR.
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 Ah you're right, thanks! I'll update that.
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
The Selenium tests are failing here because of this issue: plotly/dash-renderer#84
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 So I can remove these errors I'm throwing in favour of .isRequired and the upcoming validations?
@rmarren1
rmarren1
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
If you have .isRequired won't prop-types throw a nice exception similar to NoChildException anyway?
@valentijnnieman
valentijnnieman
commented
Oct 15, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 Yeah that's what I expected, but if I set it to .isRequired it will always throw an error, even if there are children. I think there might be an issue where Dash doesn't immediately pass the children on to the dash-renderer instance of components, or something else is going wrong in that pipeline. In the new unit tests here it's running as expected, by the way.
@valentijnnieman
valentijnnieman
changed the base branch from
update_scripts
to
masterOctober 15, 2018 15:26
@valentijnniemanvalentijnnieman
changed the title
[WIP] Unit tests with Jest + EnzymeUnit tests with Jest + Enzyme and updates to Tabs componentOct 15, 2018
@valentijnniemanvalentijnnieman
mentioned this pull request
Oct 17, 2018

Closed

valentijnnieman
valentijnnieman
commented
Oct 17, 2018
View reviewed changes
Comment threadrequirements-dev.txt
tox-pyenv
six
plotly>=2.0.8
urllib3<=1.22
@valentijnniemanvalentijnniemanOct 17, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This caused the Python 2.7 tests to fail, got this fix from #333
valentijnnieman
valentijnnieman
commented
Oct 17, 2018
View reviewed changes
Comment threadsrc/components/Tab.react.js
 * The tab's label
 */
 label: PropTypes.children,
 label: PropTypes.string,
@valentijnniemanvalentijnniemanOct 17, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Support for components/something else than a string for label will have to come later, anything else than a string doesn't really work now anyways, so best to set it to string for now.
chriddyp
chriddyp
approved these changes
Oct 17, 2018
View reviewed changes
@chriddypchriddyp
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Let's also bump the versions and ship a release as soon as this is merged
@valentijnnieman
valentijnnieman
merged commit 0c7ffb4
into
masterOct 17, 2018
@valentijnnieman
valentijnnieman
deleted the

unit_tests_with_jest

branch
October 17, 2018 20:15
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@Marc-Andre-RivetMarc-Andre-RivetAwaiting requested review from Marc-Andre-Rivet

1 more reviewer

@chriddypchriddypchriddyp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@valentijnnieman@chriddyp@rmarren1@T4rk1n

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Unit tests with Jest + Enzyme and updates to Tabs component by valentijnnieman · Pull Request #315 · plotly/dash-core-components · GitHub
Skip to content

This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Unit tests with Jest + Enzyme and updates to Tabs component - #315
Merged
valentijnnieman merged 20 commits into
masterfrom 
unit_tests_with_jest
Oct 17, 2018
Merged
Unit tests with Jest + Enzyme and updates to Tabs component#315
valentijnnieman merged 20 commits into
masterfrom 
unit_tests_with_jest
Conversation
@valentijnnieman
@valentijnniemanvalentijnnieman
commented
Sep 26, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
This is a work-in-progress PR, rebased of off #299 - that needs to be merged first.

I'm comparing this branch to the update_scripts branch, so the commits are easier to reason about.
This adds support for Jest, a JS test runner, and Enzyme, testing tools for React. It allows us to write unit tests in JS for the components, hopefully lightening the load/dependency on our Selenium integration tests, and allowing for a more test-driven approach by running Jest in watch mode or as a VS-Code/Atom plugin.
I've written some basic unit tests for the Tabs component, and caught a couple of bugs that I've also fixed.
@plotly/dash
valentijnnieman
valentijnnieman
commented
Sep 26, 2018
View reviewed changes
Comment thread.babelrc
 "plugins": ["transform-object-rest-spread", "styled-jsx/babel"]
 },
 "test": {
 "plugins": ["transform-object-rest-spread", "styled-jsx/babel-test"]
@valentijnniemanvalentijnniemanSep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Here we use (a brand new) plugin babel-test, so that Jest will use Babel and styled-jsx, but won't parse the classes to jsx-1324 or whatever, so that we can make assertions on them!
valentijnnieman
valentijnnieman
commented
Sep 26, 2018
View reviewed changes
Comment thread.circleci/config.yml
 command: |
 . venv/bin/activate
 python --version
 npm run test-unit
@valentijnniemanvalentijnniemanSep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This is the only real difference apart from formatting - running the new unit tests in CircleCI.
valentijnnieman
valentijnnieman
commented
Sep 26, 2018
View reviewed changes
Comment threadsrc/components/Tabs.react.js
Outdated

 this.NoChildrenError = {
 name: 'NoChildrenError',
 message: 'Tabs did not have any children Tab components!',
 };
@valentijnniemanvalentijnniemanSep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Not really sure about this - if you have suggestions on how to do error handling here in a better way, please let me know!
@chriddyp
chriddyp
commented
Sep 26, 2018

Copy link


Copy Markdown

Member
I've written some basic unit tests for the Tabs component, and caught a couple of bugs that I've also fixed.
Just curious, what bugs did you find that weren't covered in the integration tests?
@rmarren1
rmarren1
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
I was getting an exception when the Tabs children is empty. Does this test that case?
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I've written some basic unit tests for the Tabs component, and caught a couple of bugs that I've also fixed.
Just curious, what bugs did you find that weren't covered in the integration tests?
screen shot 2018-09-26 at 4 02 12 pm

This happens when you only have one Tab component as children
screen shot 2018-09-26 at 4 02 39 pm

I haven't tested if these are actual bugs that happen in Dash though. But at minimum these unit tests make the JS code more stable and less prone to breaking.
@T4rk1n
T4rk1n
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
I think I broke dash 0.28.0 😞
Traceback (most recent call last):
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 2292, in wsgi_app
response = self.full_dispatch_request()
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1815, in full_dispatch_request
rv = self.handle_user_exception(e)
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1718, in handle_user_exception
reraise(exc_type, exc_value, tb)
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1813, in full_dispatch_request
rv = self.dispatch_request()
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1799, in dispatch_request
return self.view_functions[rule.endpoint](**req.view_args)
File "/home/circleci/project/venv/lib/python2.7/site-packages/dash/dash.py", line 995, in _serve_default_favicon
return flask.Response(pkgutil.get_data('dash', 'favicon.ico'),
File "/usr/local/lib/python2.7/pkgutil.py", line 589, in get_data
return loader.get_data(resource_name)
File "/usr/local/lib/python2.7/pkgutil.py", line 252, in get_data
with open(pathname, "rb") as file:
IOError: [Errno 2] No such file or directory: '/home/circleci/project/venv/lib/python2.7/site-packages/dash/favicon.ico'

@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 Actually no it doesn't. I was thinking about it, and thought that the Tabs component should throw an error if there are no children. What would it render? Thinking about it now, perhaps the Tabs component should not throw an exception when there are no children, in the cases where you set the children later through a callback...
@rmarren1
rmarren1
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
I think either would be acceptable, but let's use .isRequired consistently with this choice so that users will see a nicer error via the validation PR.
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 Ah you're right, thanks! I'll update that.
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
The Selenium tests are failing here because of this issue: plotly/dash-renderer#84
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 So I can remove these errors I'm throwing in favour of .isRequired and the upcoming validations?
@rmarren1
rmarren1
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
If you have .isRequired won't prop-types throw a nice exception similar to NoChildException anyway?
@valentijnnieman
valentijnnieman
commented
Oct 15, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 Yeah that's what I expected, but if I set it to .isRequired it will always throw an error, even if there are children. I think there might be an issue where Dash doesn't immediately pass the children on to the dash-renderer instance of components, or something else is going wrong in that pipeline. In the new unit tests here it's running as expected, by the way.
@valentijnnieman
valentijnnieman
changed the base branch from
update_scripts
to
masterOctober 15, 2018 15:26
@valentijnniemanvalentijnnieman
changed the title
[WIP] Unit tests with Jest + EnzymeUnit tests with Jest + Enzyme and updates to Tabs componentOct 15, 2018
@valentijnniemanvalentijnnieman
mentioned this pull request
Oct 17, 2018

Closed

valentijnnieman
valentijnnieman
commented
Oct 17, 2018
View reviewed changes
Comment threadrequirements-dev.txt
tox-pyenv
six
plotly>=2.0.8
urllib3<=1.22
@valentijnniemanvalentijnniemanOct 17, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This caused the Python 2.7 tests to fail, got this fix from #333
valentijnnieman
valentijnnieman
commented
Oct 17, 2018
View reviewed changes
Comment threadsrc/components/Tab.react.js
 * The tab's label
 */
 label: PropTypes.children,
 label: PropTypes.string,
@valentijnniemanvalentijnniemanOct 17, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Support for components/something else than a string for label will have to come later, anything else than a string doesn't really work now anyways, so best to set it to string for now.
chriddyp
chriddyp
approved these changes
Oct 17, 2018
View reviewed changes
@chriddypchriddyp
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Let's also bump the versions and ship a release as soon as this is merged
@valentijnnieman
valentijnnieman
merged commit 0c7ffb4
into
masterOct 17, 2018
@valentijnnieman
valentijnnieman
deleted the

unit_tests_with_jest

branch
October 17, 2018 20:15
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@Marc-Andre-RivetMarc-Andre-RivetAwaiting requested review from Marc-Andre-Rivet

1 more reviewer

@chriddypchriddypchriddyp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@valentijnnieman@chriddyp@rmarren1@T4rk1n

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();
Unit tests with Jest + Enzyme and updates to Tabs component by valentijnnieman · Pull Request #315 · plotly/dash-core-components · GitHub
Skip to content

This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Unit tests with Jest + Enzyme and updates to Tabs component - #315
Merged
valentijnnieman merged 20 commits into
masterfrom 
unit_tests_with_jest
Oct 17, 2018
Merged
Unit tests with Jest + Enzyme and updates to Tabs component#315
valentijnnieman merged 20 commits into
masterfrom 
unit_tests_with_jest
Conversation
@valentijnnieman
@valentijnniemanvalentijnnieman
commented
Sep 26, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
This is a work-in-progress PR, rebased of off #299 - that needs to be merged first.

I'm comparing this branch to the update_scripts branch, so the commits are easier to reason about.
This adds support for Jest, a JS test runner, and Enzyme, testing tools for React. It allows us to write unit tests in JS for the components, hopefully lightening the load/dependency on our Selenium integration tests, and allowing for a more test-driven approach by running Jest in watch mode or as a VS-Code/Atom plugin.
I've written some basic unit tests for the Tabs component, and caught a couple of bugs that I've also fixed.
@plotly/dash
valentijnnieman
valentijnnieman
commented
Sep 26, 2018
View reviewed changes
Comment thread.babelrc
 "plugins": ["transform-object-rest-spread", "styled-jsx/babel"]
 },
 "test": {
 "plugins": ["transform-object-rest-spread", "styled-jsx/babel-test"]
@valentijnniemanvalentijnniemanSep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Here we use (a brand new) plugin babel-test, so that Jest will use Babel and styled-jsx, but won't parse the classes to jsx-1324 or whatever, so that we can make assertions on them!
valentijnnieman
valentijnnieman
commented
Sep 26, 2018
View reviewed changes
Comment thread.circleci/config.yml
 command: |
 . venv/bin/activate
 python --version
 npm run test-unit
@valentijnniemanvalentijnniemanSep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This is the only real difference apart from formatting - running the new unit tests in CircleCI.
valentijnnieman
valentijnnieman
commented
Sep 26, 2018
View reviewed changes
Comment threadsrc/components/Tabs.react.js
Outdated

 this.NoChildrenError = {
 name: 'NoChildrenError',
 message: 'Tabs did not have any children Tab components!',
 };
@valentijnniemanvalentijnniemanSep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Not really sure about this - if you have suggestions on how to do error handling here in a better way, please let me know!
@chriddyp
chriddyp
commented
Sep 26, 2018

Copy link


Copy Markdown

Member
I've written some basic unit tests for the Tabs component, and caught a couple of bugs that I've also fixed.
Just curious, what bugs did you find that weren't covered in the integration tests?
@rmarren1
rmarren1
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
I was getting an exception when the Tabs children is empty. Does this test that case?
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I've written some basic unit tests for the Tabs component, and caught a couple of bugs that I've also fixed.
Just curious, what bugs did you find that weren't covered in the integration tests?
screen shot 2018-09-26 at 4 02 12 pm

This happens when you only have one Tab component as children
screen shot 2018-09-26 at 4 02 39 pm

I haven't tested if these are actual bugs that happen in Dash though. But at minimum these unit tests make the JS code more stable and less prone to breaking.
@T4rk1n
T4rk1n
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
I think I broke dash 0.28.0 😞
Traceback (most recent call last):
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 2292, in wsgi_app
response = self.full_dispatch_request()
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1815, in full_dispatch_request
rv = self.handle_user_exception(e)
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1718, in handle_user_exception
reraise(exc_type, exc_value, tb)
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1813, in full_dispatch_request
rv = self.dispatch_request()
File "/home/circleci/project/venv/lib/python2.7/site-packages/flask/app.py", line 1799, in dispatch_request
return self.view_functions[rule.endpoint](**req.view_args)
File "/home/circleci/project/venv/lib/python2.7/site-packages/dash/dash.py", line 995, in _serve_default_favicon
return flask.Response(pkgutil.get_data('dash', 'favicon.ico'),
File "/usr/local/lib/python2.7/pkgutil.py", line 589, in get_data
return loader.get_data(resource_name)
File "/usr/local/lib/python2.7/pkgutil.py", line 252, in get_data
with open(pathname, "rb") as file:
IOError: [Errno 2] No such file or directory: '/home/circleci/project/venv/lib/python2.7/site-packages/dash/favicon.ico'

@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 Actually no it doesn't. I was thinking about it, and thought that the Tabs component should throw an error if there are no children. What would it render? Thinking about it now, perhaps the Tabs component should not throw an exception when there are no children, in the cases where you set the children later through a callback...
@rmarren1
rmarren1
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
I think either would be acceptable, but let's use .isRequired consistently with this choice so that users will see a nicer error via the validation PR.
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 Ah you're right, thanks! I'll update that.
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
The Selenium tests are failing here because of this issue: plotly/dash-renderer#84
@valentijnnieman
valentijnnieman
commented
Sep 26, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 So I can remove these errors I'm throwing in favour of .isRequired and the upcoming validations?
@rmarren1
rmarren1
commented
Sep 26, 2018

Copy link


Copy Markdown

Contributor
If you have .isRequired won't prop-types throw a nice exception similar to NoChildException anyway?
@valentijnnieman
valentijnnieman
commented
Oct 15, 2018

Copy link


Copy Markdown

ContributorAuthor
@rmarren1 Yeah that's what I expected, but if I set it to .isRequired it will always throw an error, even if there are children. I think there might be an issue where Dash doesn't immediately pass the children on to the dash-renderer instance of components, or something else is going wrong in that pipeline. In the new unit tests here it's running as expected, by the way.
@valentijnnieman
valentijnnieman
changed the base branch from
update_scripts
to
masterOctober 15, 2018 15:26
@valentijnniemanvalentijnnieman
changed the title
[WIP] Unit tests with Jest + EnzymeUnit tests with Jest + Enzyme and updates to Tabs componentOct 15, 2018
@valentijnniemanvalentijnnieman
mentioned this pull request
Oct 17, 2018

Closed

valentijnnieman
valentijnnieman
commented
Oct 17, 2018
View reviewed changes
Comment threadrequirements-dev.txt
tox-pyenv
six
plotly>=2.0.8
urllib3<=1.22
@valentijnniemanvalentijnniemanOct 17, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This caused the Python 2.7 tests to fail, got this fix from #333
valentijnnieman
valentijnnieman
commented
Oct 17, 2018
View reviewed changes
Comment threadsrc/components/Tab.react.js
 * The tab's label
 */
 label: PropTypes.children,
 label: PropTypes.string,
@valentijnniemanvalentijnniemanOct 17, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Support for components/something else than a string for label will have to come later, anything else than a string doesn't really work now anyways, so best to set it to string for now.
chriddyp
chriddyp
approved these changes
Oct 17, 2018
View reviewed changes
@chriddypchriddyp
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Let's also bump the versions and ship a release as soon as this is merged
@valentijnnieman
valentijnnieman
merged commit 0c7ffb4
into
masterOct 17, 2018
@valentijnnieman
valentijnnieman
deleted the

unit_tests_with_jest

branch
October 17, 2018 20:15
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@Marc-Andre-RivetMarc-Andre-RivetAwaiting requested review from Marc-Andre-Rivet

1 more reviewer

@chriddypchriddypchriddyp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@valentijnnieman@chriddyp@rmarren1@T4rk1n