Loosen testing requirements & improve testing - #1506

Merged
alexcjohnson merged 11 commits into
devfrom
loosen-testing-reqs
Dec 17, 2020
Merged

Loosen testing requirements & improve testing#1506
alexcjohnson merged 11 commits into
devfrom
loosen-testing-reqs

Conversation

@alexcjohnson

@alexcjohnsonalexcjohnson commented Dec 17, 2020

Copy link
Copy Markdown
Collaborator

Fixes#1466 - loosens all dash[testing] requirements to >= for better compatibility with outside projects.
In addition I made two other changes:

  • Bumped various dash[dev] requirements. I had thought that this was only used by us internally, on CI, but in fact it's required for building components, and as such is referenced in the component boilerplate. So we may need to revisit this.
  • Something in that process broke running our old-style locally, though they still appear to run on CI. I took this opportunity to convert the rest of them to dash.testing and getting rid of the last unittest style tests.

Contributor Checklist

  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added entry in the CHANGELOG.md

Comment threadrequires-dev.txt
virtualenv==20.2.2;python_version=="2.7"
fire==0.3.1
coloredlogs==15.0
flask-talisman==0.7.0

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Moved flask-talisman here from requires-testing because this is used in our tests themselves, not in dash.testing.

Comment threadrequires-testing.txt
selenium>=3.141.0
percy>=2.0.2
requests[security]>=2.21.0
beautifulsoup4>=4.8.2,<=4.9.3;python_version=="2.7"

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

beautifulsoup4 has stated it's about to drop Py2 support, so I set a preemptive upper bound for Py2 at the current version.

assert "dash_core_components" in ComponentRegistry.registry
assert "dash_html_components" in ComponentRegistry.registry
mocker.patch("dash.development.base_component.ComponentRegistry.registry")
ComponentRegistry.registry = {"dash_core_components", "dash_html_components"}

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

These tests would fail previously if run in tandem with the integration tests, because they import and register various other component packages. After this change, I can simply call pytest locally to run all the Python-based tests in this repo.

There's still one that fails locally for me: The iframe sandbox test rdif001 fails on its last line - it can't see the log error it's supposed to have. I'm not sure what's going on here but it seems we've run afoul of some sort of security restriction: in the Chrome devtools I can't even see the DOM inside the iframe (though I can interact with this DOM via Selenium).
Screen Shot 2020-12-16 at 9 42 59 PM
Anyway this test still runs fine on CI, but if what I'm seeing is new behavior in the latest Chrome (v87) we may need to sort this out soon.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Running this locally on Chrome 87, getting:
image

Running this locally on Chrome 89 (Canary), getting:
image

Moving from warning to warning+error.

Changing the iframe sandbox options to

<iframe src="{0}" sandbox="allow-scripts allow-same-origin">
  • fixes the problem on Chrome 87
  • fixes the warning on Chrome 89 but the error remains
    image

Attempting to find additional information.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In both Chrome 87 and 89 localStorage is not accessible from data URL but in 89 it's shown from the start. In both 87/89 running window.localStorage from the console gives
image

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

To be clear, the test requires that a warning is logged so we're not trying to "fix" all the warnings. I'm not sure it's really important that we test for these logs, the main thing is that the app still works and doesn't error out trying to access cookies. So the easy answer here would be to simply remove the log check. But the "full" solution I guess would be to serve a real container page with an iframe in it, rather than a data: url, and keep the check as is.

For reference this feature was introduced in #1080

Comment threadrequires-dev.txt
flake8==3.8.4
PyYAML==5.3.1
pylint==1.9.5;python_version<"3.7"
pylint==2.6.0;python_version>="3.7"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we sanity test these flake8/pylint changes against the other core repos?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

They have an annoying habit of changing their rules..

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Good call - they may well need their own updates to .pylintrc files, or syntax updates. PRs coming.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

plotly/dash-core-components#907, plotly/dash-html-components#171, plotly/dash-table#857 - All three, I started with this branch, got tests passing, then reverted to the dev branch of dash and tests still pass - All that was required (other than prohibiting the latest xlrd) was a couple of linting exclusions.

Comment threadtests/integration/callbacks/test_multiple_callbacks.py
Comment threadtests/integration/renderer/test_due_diligence.py Outdated
@alexcjohnson
alexcjohnson merged commit 8a4873d into devDec 17, 2020
@alexcjohnson
alexcjohnson deleted the loosen-testing-reqs branch December 17, 2020 20:20
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Loosen or update cryptography requirement for dash[testing]

2 participants

@alexcjohnson@Marc-Andre-Rivet
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n 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;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Loosen testing requirements & improve testing - #1506

Merged
alexcjohnson merged 11 commits into
devfrom
loosen-testing-reqs
Dec 17, 2020
Merged

Loosen testing requirements & improve testing#1506
alexcjohnson merged 11 commits into
devfrom
loosen-testing-reqs

Conversation

@alexcjohnson

@alexcjohnsonalexcjohnson commented Dec 17, 2020

Copy link
Copy Markdown
Collaborator

Fixes#1466 - loosens all dash[testing] requirements to >= for better compatibility with outside projects.
In addition I made two other changes:

  • Bumped various dash[dev] requirements. I had thought that this was only used by us internally, on CI, but in fact it's required for building components, and as such is referenced in the component boilerplate. So we may need to revisit this.
  • Something in that process broke running our old-style locally, though they still appear to run on CI. I took this opportunity to convert the rest of them to dash.testing and getting rid of the last unittest style tests.

Contributor Checklist

  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added entry in the CHANGELOG.md

Comment threadrequires-dev.txt
virtualenv==20.2.2;python_version=="2.7"
fire==0.3.1
coloredlogs==15.0
flask-talisman==0.7.0

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Moved flask-talisman here from requires-testing because this is used in our tests themselves, not in dash.testing.

Comment threadrequires-testing.txt
selenium>=3.141.0
percy>=2.0.2
requests[security]>=2.21.0
beautifulsoup4>=4.8.2,<=4.9.3;python_version=="2.7"

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

beautifulsoup4 has stated it's about to drop Py2 support, so I set a preemptive upper bound for Py2 at the current version.

assert "dash_core_components" in ComponentRegistry.registry
assert "dash_html_components" in ComponentRegistry.registry
mocker.patch("dash.development.base_component.ComponentRegistry.registry")
ComponentRegistry.registry = {"dash_core_components", "dash_html_components"}

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

These tests would fail previously if run in tandem with the integration tests, because they import and register various other component packages. After this change, I can simply call pytest locally to run all the Python-based tests in this repo.

There's still one that fails locally for me: The iframe sandbox test rdif001 fails on its last line - it can't see the log error it's supposed to have. I'm not sure what's going on here but it seems we've run afoul of some sort of security restriction: in the Chrome devtools I can't even see the DOM inside the iframe (though I can interact with this DOM via Selenium).
Screen Shot 2020-12-16 at 9 42 59 PM
Anyway this test still runs fine on CI, but if what I'm seeing is new behavior in the latest Chrome (v87) we may need to sort this out soon.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Running this locally on Chrome 87, getting:
image

Running this locally on Chrome 89 (Canary), getting:
image

Moving from warning to warning+error.

Changing the iframe sandbox options to

<iframe src="{0}" sandbox="allow-scripts allow-same-origin">
  • fixes the problem on Chrome 87
  • fixes the warning on Chrome 89 but the error remains
    image

Attempting to find additional information.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In both Chrome 87 and 89 localStorage is not accessible from data URL but in 89 it's shown from the start. In both 87/89 running window.localStorage from the console gives
image

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

To be clear, the test requires that a warning is logged so we're not trying to "fix" all the warnings. I'm not sure it's really important that we test for these logs, the main thing is that the app still works and doesn't error out trying to access cookies. So the easy answer here would be to simply remove the log check. But the "full" solution I guess would be to serve a real container page with an iframe in it, rather than a data: url, and keep the check as is.

For reference this feature was introduced in #1080

Comment threadrequires-dev.txt
flake8==3.8.4
PyYAML==5.3.1
pylint==1.9.5;python_version<"3.7"
pylint==2.6.0;python_version>="3.7"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we sanity test these flake8/pylint changes against the other core repos?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

They have an annoying habit of changing their rules..

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Good call - they may well need their own updates to .pylintrc files, or syntax updates. PRs coming.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

plotly/dash-core-components#907, plotly/dash-html-components#171, plotly/dash-table#857 - All three, I started with this branch, got tests passing, then reverted to the dev branch of dash and tests still pass - All that was required (other than prohibiting the latest xlrd) was a couple of linting exclusions.

Comment threadtests/integration/callbacks/test_multiple_callbacks.py
Comment threadtests/integration/renderer/test_due_diligence.py Outdated
@alexcjohnson
alexcjohnson merged commit 8a4873d into devDec 17, 2020
@alexcjohnson
alexcjohnson deleted the loosen-testing-reqs branch December 17, 2020 20:20
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Loosen or update cryptography requirement for dash[testing]

2 participants

@alexcjohnson@Marc-Andre-Rivet
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Loosen testing requirements & improve testing - #1506

Merged
alexcjohnson merged 11 commits into
devfrom
loosen-testing-reqs
Dec 17, 2020
Merged

Loosen testing requirements & improve testing#1506
alexcjohnson merged 11 commits into
devfrom
loosen-testing-reqs

Conversation

@alexcjohnson

@alexcjohnsonalexcjohnson commented Dec 17, 2020

Copy link
Copy Markdown
Collaborator

Fixes#1466 - loosens all dash[testing] requirements to >= for better compatibility with outside projects.
In addition I made two other changes:

  • Bumped various dash[dev] requirements. I had thought that this was only used by us internally, on CI, but in fact it's required for building components, and as such is referenced in the component boilerplate. So we may need to revisit this.
  • Something in that process broke running our old-style locally, though they still appear to run on CI. I took this opportunity to convert the rest of them to dash.testing and getting rid of the last unittest style tests.

Contributor Checklist

  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added entry in the CHANGELOG.md

Comment threadrequires-dev.txt
virtualenv==20.2.2;python_version=="2.7"
fire==0.3.1
coloredlogs==15.0
flask-talisman==0.7.0

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Moved flask-talisman here from requires-testing because this is used in our tests themselves, not in dash.testing.

Comment threadrequires-testing.txt
selenium>=3.141.0
percy>=2.0.2
requests[security]>=2.21.0
beautifulsoup4>=4.8.2,<=4.9.3;python_version=="2.7"

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

beautifulsoup4 has stated it's about to drop Py2 support, so I set a preemptive upper bound for Py2 at the current version.

assert "dash_core_components" in ComponentRegistry.registry
assert "dash_html_components" in ComponentRegistry.registry
mocker.patch("dash.development.base_component.ComponentRegistry.registry")
ComponentRegistry.registry = {"dash_core_components", "dash_html_components"}

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

These tests would fail previously if run in tandem with the integration tests, because they import and register various other component packages. After this change, I can simply call pytest locally to run all the Python-based tests in this repo.

There's still one that fails locally for me: The iframe sandbox test rdif001 fails on its last line - it can't see the log error it's supposed to have. I'm not sure what's going on here but it seems we've run afoul of some sort of security restriction: in the Chrome devtools I can't even see the DOM inside the iframe (though I can interact with this DOM via Selenium).
Screen Shot 2020-12-16 at 9 42 59 PM
Anyway this test still runs fine on CI, but if what I'm seeing is new behavior in the latest Chrome (v87) we may need to sort this out soon.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Running this locally on Chrome 87, getting:
image

Running this locally on Chrome 89 (Canary), getting:
image

Moving from warning to warning+error.

Changing the iframe sandbox options to

<iframe src="{0}" sandbox="allow-scripts allow-same-origin">
  • fixes the problem on Chrome 87
  • fixes the warning on Chrome 89 but the error remains
    image

Attempting to find additional information.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In both Chrome 87 and 89 localStorage is not accessible from data URL but in 89 it's shown from the start. In both 87/89 running window.localStorage from the console gives
image

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

To be clear, the test requires that a warning is logged so we're not trying to "fix" all the warnings. I'm not sure it's really important that we test for these logs, the main thing is that the app still works and doesn't error out trying to access cookies. So the easy answer here would be to simply remove the log check. But the "full" solution I guess would be to serve a real container page with an iframe in it, rather than a data: url, and keep the check as is.

For reference this feature was introduced in #1080

Comment threadrequires-dev.txt
flake8==3.8.4
PyYAML==5.3.1
pylint==1.9.5;python_version<"3.7"
pylint==2.6.0;python_version>="3.7"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we sanity test these flake8/pylint changes against the other core repos?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

They have an annoying habit of changing their rules..

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Good call - they may well need their own updates to .pylintrc files, or syntax updates. PRs coming.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

plotly/dash-core-components#907, plotly/dash-html-components#171, plotly/dash-table#857 - All three, I started with this branch, got tests passing, then reverted to the dev branch of dash and tests still pass - All that was required (other than prohibiting the latest xlrd) was a couple of linting exclusions.

Comment threadtests/integration/callbacks/test_multiple_callbacks.py
Comment threadtests/integration/renderer/test_due_diligence.py Outdated
@alexcjohnson
alexcjohnson merged commit 8a4873d into devDec 17, 2020
@alexcjohnson
alexcjohnson deleted the loosen-testing-reqs branch December 17, 2020 20:20
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Loosen or update cryptography requirement for dash[testing]

2 participants

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

Loosen testing requirements & improve testing - #1506

Merged
alexcjohnson merged 11 commits into
devfrom
loosen-testing-reqs
Dec 17, 2020
Merged

Loosen testing requirements & improve testing#1506
alexcjohnson merged 11 commits into
devfrom
loosen-testing-reqs

Conversation

@alexcjohnson

@alexcjohnsonalexcjohnson commented Dec 17, 2020

Copy link
Copy Markdown
Collaborator

Fixes#1466 - loosens all dash[testing] requirements to >= for better compatibility with outside projects.
In addition I made two other changes:

  • Bumped various dash[dev] requirements. I had thought that this was only used by us internally, on CI, but in fact it's required for building components, and as such is referenced in the component boilerplate. So we may need to revisit this.
  • Something in that process broke running our old-style locally, though they still appear to run on CI. I took this opportunity to convert the rest of them to dash.testing and getting rid of the last unittest style tests.

Contributor Checklist

  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added entry in the CHANGELOG.md

Comment threadrequires-dev.txt
virtualenv==20.2.2;python_version=="2.7"
fire==0.3.1
coloredlogs==15.0
flask-talisman==0.7.0

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Moved flask-talisman here from requires-testing because this is used in our tests themselves, not in dash.testing.

Comment threadrequires-testing.txt
selenium>=3.141.0
percy>=2.0.2
requests[security]>=2.21.0
beautifulsoup4>=4.8.2,<=4.9.3;python_version=="2.7"

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

beautifulsoup4 has stated it's about to drop Py2 support, so I set a preemptive upper bound for Py2 at the current version.

assert "dash_core_components" in ComponentRegistry.registry
assert "dash_html_components" in ComponentRegistry.registry
mocker.patch("dash.development.base_component.ComponentRegistry.registry")
ComponentRegistry.registry = {"dash_core_components", "dash_html_components"}

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

These tests would fail previously if run in tandem with the integration tests, because they import and register various other component packages. After this change, I can simply call pytest locally to run all the Python-based tests in this repo.

There's still one that fails locally for me: The iframe sandbox test rdif001 fails on its last line - it can't see the log error it's supposed to have. I'm not sure what's going on here but it seems we've run afoul of some sort of security restriction: in the Chrome devtools I can't even see the DOM inside the iframe (though I can interact with this DOM via Selenium).
Screen Shot 2020-12-16 at 9 42 59 PM
Anyway this test still runs fine on CI, but if what I'm seeing is new behavior in the latest Chrome (v87) we may need to sort this out soon.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Running this locally on Chrome 87, getting:
image

Running this locally on Chrome 89 (Canary), getting:
image

Moving from warning to warning+error.

Changing the iframe sandbox options to

<iframe src="{0}" sandbox="allow-scripts allow-same-origin">
  • fixes the problem on Chrome 87
  • fixes the warning on Chrome 89 but the error remains
    image

Attempting to find additional information.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In both Chrome 87 and 89 localStorage is not accessible from data URL but in 89 it's shown from the start. In both 87/89 running window.localStorage from the console gives
image

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

To be clear, the test requires that a warning is logged so we're not trying to "fix" all the warnings. I'm not sure it's really important that we test for these logs, the main thing is that the app still works and doesn't error out trying to access cookies. So the easy answer here would be to simply remove the log check. But the "full" solution I guess would be to serve a real container page with an iframe in it, rather than a data: url, and keep the check as is.

For reference this feature was introduced in #1080

Comment threadrequires-dev.txt
flake8==3.8.4
PyYAML==5.3.1
pylint==1.9.5;python_version<"3.7"
pylint==2.6.0;python_version>="3.7"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we sanity test these flake8/pylint changes against the other core repos?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

They have an annoying habit of changing their rules..

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Good call - they may well need their own updates to .pylintrc files, or syntax updates. PRs coming.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

plotly/dash-core-components#907, plotly/dash-html-components#171, plotly/dash-table#857 - All three, I started with this branch, got tests passing, then reverted to the dev branch of dash and tests still pass - All that was required (other than prohibiting the latest xlrd) was a couple of linting exclusions.

Comment threadtests/integration/callbacks/test_multiple_callbacks.py
Comment threadtests/integration/renderer/test_due_diligence.py Outdated
@alexcjohnson
alexcjohnson merged commit 8a4873d into devDec 17, 2020
@alexcjohnson
alexcjohnson deleted the loosen-testing-reqs branch December 17, 2020 20:20
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Loosen or update cryptography requirement for dash[testing]

2 participants

@alexcjohnson@Marc-Andre-Rivet
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Loosen testing requirements & improve testing - #1506

Merged
alexcjohnson merged 11 commits into
devfrom
loosen-testing-reqs
Dec 17, 2020
Merged

Loosen testing requirements & improve testing#1506
alexcjohnson merged 11 commits into
devfrom
loosen-testing-reqs

Conversation

@alexcjohnson

@alexcjohnsonalexcjohnson commented Dec 17, 2020

Copy link
Copy Markdown
Collaborator

Fixes#1466 - loosens all dash[testing] requirements to >= for better compatibility with outside projects.
In addition I made two other changes:

  • Bumped various dash[dev] requirements. I had thought that this was only used by us internally, on CI, but in fact it's required for building components, and as such is referenced in the component boilerplate. So we may need to revisit this.
  • Something in that process broke running our old-style locally, though they still appear to run on CI. I took this opportunity to convert the rest of them to dash.testing and getting rid of the last unittest style tests.

Contributor Checklist

  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added entry in the CHANGELOG.md

Comment threadrequires-dev.txt
virtualenv==20.2.2;python_version=="2.7"
fire==0.3.1
coloredlogs==15.0
flask-talisman==0.7.0

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Moved flask-talisman here from requires-testing because this is used in our tests themselves, not in dash.testing.

Comment threadrequires-testing.txt
selenium>=3.141.0
percy>=2.0.2
requests[security]>=2.21.0
beautifulsoup4>=4.8.2,<=4.9.3;python_version=="2.7"

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

beautifulsoup4 has stated it's about to drop Py2 support, so I set a preemptive upper bound for Py2 at the current version.

assert "dash_core_components" in ComponentRegistry.registry
assert "dash_html_components" in ComponentRegistry.registry
mocker.patch("dash.development.base_component.ComponentRegistry.registry")
ComponentRegistry.registry = {"dash_core_components", "dash_html_components"}

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

These tests would fail previously if run in tandem with the integration tests, because they import and register various other component packages. After this change, I can simply call pytest locally to run all the Python-based tests in this repo.

There's still one that fails locally for me: The iframe sandbox test rdif001 fails on its last line - it can't see the log error it's supposed to have. I'm not sure what's going on here but it seems we've run afoul of some sort of security restriction: in the Chrome devtools I can't even see the DOM inside the iframe (though I can interact with this DOM via Selenium).
Screen Shot 2020-12-16 at 9 42 59 PM
Anyway this test still runs fine on CI, but if what I'm seeing is new behavior in the latest Chrome (v87) we may need to sort this out soon.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Running this locally on Chrome 87, getting:
image

Running this locally on Chrome 89 (Canary), getting:
image

Moving from warning to warning+error.

Changing the iframe sandbox options to

<iframe src="{0}" sandbox="allow-scripts allow-same-origin">
  • fixes the problem on Chrome 87
  • fixes the warning on Chrome 89 but the error remains
    image

Attempting to find additional information.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In both Chrome 87 and 89 localStorage is not accessible from data URL but in 89 it's shown from the start. In both 87/89 running window.localStorage from the console gives
image

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

To be clear, the test requires that a warning is logged so we're not trying to "fix" all the warnings. I'm not sure it's really important that we test for these logs, the main thing is that the app still works and doesn't error out trying to access cookies. So the easy answer here would be to simply remove the log check. But the "full" solution I guess would be to serve a real container page with an iframe in it, rather than a data: url, and keep the check as is.

For reference this feature was introduced in #1080

Comment threadrequires-dev.txt
flake8==3.8.4
PyYAML==5.3.1
pylint==1.9.5;python_version<"3.7"
pylint==2.6.0;python_version>="3.7"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we sanity test these flake8/pylint changes against the other core repos?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

They have an annoying habit of changing their rules..

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Good call - they may well need their own updates to .pylintrc files, or syntax updates. PRs coming.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

plotly/dash-core-components#907, plotly/dash-html-components#171, plotly/dash-table#857 - All three, I started with this branch, got tests passing, then reverted to the dev branch of dash and tests still pass - All that was required (other than prohibiting the latest xlrd) was a couple of linting exclusions.

Comment threadtests/integration/callbacks/test_multiple_callbacks.py
Comment threadtests/integration/renderer/test_due_diligence.py Outdated
@alexcjohnson
alexcjohnson merged commit 8a4873d into devDec 17, 2020
@alexcjohnson
alexcjohnson deleted the loosen-testing-reqs branch December 17, 2020 20:20
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Loosen or update cryptography requirement for dash[testing]

2 participants

@alexcjohnson@Marc-Andre-Rivet
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Loosen testing requirements & improve testing - #1506

Merged
alexcjohnson merged 11 commits into
devfrom
loosen-testing-reqs
Dec 17, 2020
Merged

Loosen testing requirements & improve testing#1506
alexcjohnson merged 11 commits into
devfrom
loosen-testing-reqs

Conversation

@alexcjohnson

@alexcjohnsonalexcjohnson commented Dec 17, 2020

Copy link
Copy Markdown
Collaborator

Fixes#1466 - loosens all dash[testing] requirements to >= for better compatibility with outside projects.
In addition I made two other changes:

  • Bumped various dash[dev] requirements. I had thought that this was only used by us internally, on CI, but in fact it's required for building components, and as such is referenced in the component boilerplate. So we may need to revisit this.
  • Something in that process broke running our old-style locally, though they still appear to run on CI. I took this opportunity to convert the rest of them to dash.testing and getting rid of the last unittest style tests.

Contributor Checklist

  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added entry in the CHANGELOG.md

Comment threadrequires-dev.txt
virtualenv==20.2.2;python_version=="2.7"
fire==0.3.1
coloredlogs==15.0
flask-talisman==0.7.0

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Moved flask-talisman here from requires-testing because this is used in our tests themselves, not in dash.testing.

Comment threadrequires-testing.txt
selenium>=3.141.0
percy>=2.0.2
requests[security]>=2.21.0
beautifulsoup4>=4.8.2,<=4.9.3;python_version=="2.7"

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

beautifulsoup4 has stated it's about to drop Py2 support, so I set a preemptive upper bound for Py2 at the current version.

assert "dash_core_components" in ComponentRegistry.registry
assert "dash_html_components" in ComponentRegistry.registry
mocker.patch("dash.development.base_component.ComponentRegistry.registry")
ComponentRegistry.registry = {"dash_core_components", "dash_html_components"}

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

These tests would fail previously if run in tandem with the integration tests, because they import and register various other component packages. After this change, I can simply call pytest locally to run all the Python-based tests in this repo.

There's still one that fails locally for me: The iframe sandbox test rdif001 fails on its last line - it can't see the log error it's supposed to have. I'm not sure what's going on here but it seems we've run afoul of some sort of security restriction: in the Chrome devtools I can't even see the DOM inside the iframe (though I can interact with this DOM via Selenium).
Screen Shot 2020-12-16 at 9 42 59 PM
Anyway this test still runs fine on CI, but if what I'm seeing is new behavior in the latest Chrome (v87) we may need to sort this out soon.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Running this locally on Chrome 87, getting:
image

Running this locally on Chrome 89 (Canary), getting:
image

Moving from warning to warning+error.

Changing the iframe sandbox options to

<iframe src="{0}" sandbox="allow-scripts allow-same-origin">
  • fixes the problem on Chrome 87
  • fixes the warning on Chrome 89 but the error remains
    image

Attempting to find additional information.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In both Chrome 87 and 89 localStorage is not accessible from data URL but in 89 it's shown from the start. In both 87/89 running window.localStorage from the console gives
image

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

To be clear, the test requires that a warning is logged so we're not trying to "fix" all the warnings. I'm not sure it's really important that we test for these logs, the main thing is that the app still works and doesn't error out trying to access cookies. So the easy answer here would be to simply remove the log check. But the "full" solution I guess would be to serve a real container page with an iframe in it, rather than a data: url, and keep the check as is.

For reference this feature was introduced in #1080

Comment threadrequires-dev.txt
flake8==3.8.4
PyYAML==5.3.1
pylint==1.9.5;python_version<"3.7"
pylint==2.6.0;python_version>="3.7"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we sanity test these flake8/pylint changes against the other core repos?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

They have an annoying habit of changing their rules..

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Good call - they may well need their own updates to .pylintrc files, or syntax updates. PRs coming.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

plotly/dash-core-components#907, plotly/dash-html-components#171, plotly/dash-table#857 - All three, I started with this branch, got tests passing, then reverted to the dev branch of dash and tests still pass - All that was required (other than prohibiting the latest xlrd) was a couple of linting exclusions.

Comment threadtests/integration/callbacks/test_multiple_callbacks.py
Comment threadtests/integration/renderer/test_due_diligence.py Outdated
@alexcjohnson
alexcjohnson merged commit 8a4873d into devDec 17, 2020
@alexcjohnson
alexcjohnson deleted the loosen-testing-reqs branch December 17, 2020 20:20
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Loosen or update cryptography requirement for dash[testing]

2 participants

@alexcjohnson@Marc-Andre-Rivet
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Loosen testing requirements & improve testing - #1506

Merged
alexcjohnson merged 11 commits into
devfrom
loosen-testing-reqs
Dec 17, 2020
Merged

Loosen testing requirements & improve testing#1506
alexcjohnson merged 11 commits into
devfrom
loosen-testing-reqs

Conversation

@alexcjohnson

@alexcjohnsonalexcjohnson commented Dec 17, 2020

Copy link
Copy Markdown
Collaborator

Fixes#1466 - loosens all dash[testing] requirements to >= for better compatibility with outside projects.
In addition I made two other changes:

  • Bumped various dash[dev] requirements. I had thought that this was only used by us internally, on CI, but in fact it's required for building components, and as such is referenced in the component boilerplate. So we may need to revisit this.
  • Something in that process broke running our old-style locally, though they still appear to run on CI. I took this opportunity to convert the rest of them to dash.testing and getting rid of the last unittest style tests.

Contributor Checklist

  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added entry in the CHANGELOG.md

Comment threadrequires-dev.txt
virtualenv==20.2.2;python_version=="2.7"
fire==0.3.1
coloredlogs==15.0
flask-talisman==0.7.0

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Moved flask-talisman here from requires-testing because this is used in our tests themselves, not in dash.testing.

Comment threadrequires-testing.txt
selenium>=3.141.0
percy>=2.0.2
requests[security]>=2.21.0
beautifulsoup4>=4.8.2,<=4.9.3;python_version=="2.7"

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

beautifulsoup4 has stated it's about to drop Py2 support, so I set a preemptive upper bound for Py2 at the current version.

assert "dash_core_components" in ComponentRegistry.registry
assert "dash_html_components" in ComponentRegistry.registry
mocker.patch("dash.development.base_component.ComponentRegistry.registry")
ComponentRegistry.registry = {"dash_core_components", "dash_html_components"}

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

These tests would fail previously if run in tandem with the integration tests, because they import and register various other component packages. After this change, I can simply call pytest locally to run all the Python-based tests in this repo.

There's still one that fails locally for me: The iframe sandbox test rdif001 fails on its last line - it can't see the log error it's supposed to have. I'm not sure what's going on here but it seems we've run afoul of some sort of security restriction: in the Chrome devtools I can't even see the DOM inside the iframe (though I can interact with this DOM via Selenium).
Screen Shot 2020-12-16 at 9 42 59 PM
Anyway this test still runs fine on CI, but if what I'm seeing is new behavior in the latest Chrome (v87) we may need to sort this out soon.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Running this locally on Chrome 87, getting:
image

Running this locally on Chrome 89 (Canary), getting:
image

Moving from warning to warning+error.

Changing the iframe sandbox options to

<iframe src="{0}" sandbox="allow-scripts allow-same-origin">
  • fixes the problem on Chrome 87
  • fixes the warning on Chrome 89 but the error remains
    image

Attempting to find additional information.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In both Chrome 87 and 89 localStorage is not accessible from data URL but in 89 it's shown from the start. In both 87/89 running window.localStorage from the console gives
image

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

To be clear, the test requires that a warning is logged so we're not trying to "fix" all the warnings. I'm not sure it's really important that we test for these logs, the main thing is that the app still works and doesn't error out trying to access cookies. So the easy answer here would be to simply remove the log check. But the "full" solution I guess would be to serve a real container page with an iframe in it, rather than a data: url, and keep the check as is.

For reference this feature was introduced in #1080

Comment threadrequires-dev.txt
flake8==3.8.4
PyYAML==5.3.1
pylint==1.9.5;python_version<"3.7"
pylint==2.6.0;python_version>="3.7"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we sanity test these flake8/pylint changes against the other core repos?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

They have an annoying habit of changing their rules..

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Good call - they may well need their own updates to .pylintrc files, or syntax updates. PRs coming.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

plotly/dash-core-components#907, plotly/dash-html-components#171, plotly/dash-table#857 - All three, I started with this branch, got tests passing, then reverted to the dev branch of dash and tests still pass - All that was required (other than prohibiting the latest xlrd) was a couple of linting exclusions.

Comment threadtests/integration/callbacks/test_multiple_callbacks.py
Comment threadtests/integration/renderer/test_due_diligence.py Outdated
@alexcjohnson
alexcjohnson merged commit 8a4873d into devDec 17, 2020
@alexcjohnson
alexcjohnson deleted the loosen-testing-reqs branch December 17, 2020 20:20
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Loosen or update cryptography requirement for dash[testing]

2 participants

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

Loosen testing requirements & improve testing - #1506

Merged
alexcjohnson merged 11 commits into
devfrom
loosen-testing-reqs
Dec 17, 2020
Merged

Loosen testing requirements & improve testing#1506
alexcjohnson merged 11 commits into
devfrom
loosen-testing-reqs

Conversation

@alexcjohnson

@alexcjohnsonalexcjohnson commented Dec 17, 2020

Copy link
Copy Markdown
Collaborator

Fixes#1466 - loosens all dash[testing] requirements to >= for better compatibility with outside projects.
In addition I made two other changes:

  • Bumped various dash[dev] requirements. I had thought that this was only used by us internally, on CI, but in fact it's required for building components, and as such is referenced in the component boilerplate. So we may need to revisit this.
  • Something in that process broke running our old-style locally, though they still appear to run on CI. I took this opportunity to convert the rest of them to dash.testing and getting rid of the last unittest style tests.

Contributor Checklist

  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added entry in the CHANGELOG.md

Comment threadrequires-dev.txt
virtualenv==20.2.2;python_version=="2.7"
fire==0.3.1
coloredlogs==15.0
flask-talisman==0.7.0

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Moved flask-talisman here from requires-testing because this is used in our tests themselves, not in dash.testing.

Comment threadrequires-testing.txt
selenium>=3.141.0
percy>=2.0.2
requests[security]>=2.21.0
beautifulsoup4>=4.8.2,<=4.9.3;python_version=="2.7"

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

beautifulsoup4 has stated it's about to drop Py2 support, so I set a preemptive upper bound for Py2 at the current version.

assert "dash_core_components" in ComponentRegistry.registry
assert "dash_html_components" in ComponentRegistry.registry
mocker.patch("dash.development.base_component.ComponentRegistry.registry")
ComponentRegistry.registry = {"dash_core_components", "dash_html_components"}

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

These tests would fail previously if run in tandem with the integration tests, because they import and register various other component packages. After this change, I can simply call pytest locally to run all the Python-based tests in this repo.

There's still one that fails locally for me: The iframe sandbox test rdif001 fails on its last line - it can't see the log error it's supposed to have. I'm not sure what's going on here but it seems we've run afoul of some sort of security restriction: in the Chrome devtools I can't even see the DOM inside the iframe (though I can interact with this DOM via Selenium).
Screen Shot 2020-12-16 at 9 42 59 PM
Anyway this test still runs fine on CI, but if what I'm seeing is new behavior in the latest Chrome (v87) we may need to sort this out soon.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Running this locally on Chrome 87, getting:
image

Running this locally on Chrome 89 (Canary), getting:
image

Moving from warning to warning+error.

Changing the iframe sandbox options to

<iframe src="{0}" sandbox="allow-scripts allow-same-origin">
  • fixes the problem on Chrome 87
  • fixes the warning on Chrome 89 but the error remains
    image

Attempting to find additional information.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In both Chrome 87 and 89 localStorage is not accessible from data URL but in 89 it's shown from the start. In both 87/89 running window.localStorage from the console gives
image

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

To be clear, the test requires that a warning is logged so we're not trying to "fix" all the warnings. I'm not sure it's really important that we test for these logs, the main thing is that the app still works and doesn't error out trying to access cookies. So the easy answer here would be to simply remove the log check. But the "full" solution I guess would be to serve a real container page with an iframe in it, rather than a data: url, and keep the check as is.

For reference this feature was introduced in #1080

Comment threadrequires-dev.txt
flake8==3.8.4
PyYAML==5.3.1
pylint==1.9.5;python_version<"3.7"
pylint==2.6.0;python_version>="3.7"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we sanity test these flake8/pylint changes against the other core repos?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

They have an annoying habit of changing their rules..

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Good call - they may well need their own updates to .pylintrc files, or syntax updates. PRs coming.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

plotly/dash-core-components#907, plotly/dash-html-components#171, plotly/dash-table#857 - All three, I started with this branch, got tests passing, then reverted to the dev branch of dash and tests still pass - All that was required (other than prohibiting the latest xlrd) was a couple of linting exclusions.

Comment threadtests/integration/callbacks/test_multiple_callbacks.py
Comment threadtests/integration/renderer/test_due_diligence.py Outdated
@alexcjohnson
alexcjohnson merged commit 8a4873d into devDec 17, 2020
@alexcjohnson
alexcjohnson deleted the loosen-testing-reqs branch December 17, 2020 20:20
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Loosen or update cryptography requirement for dash[testing]

2 participants

@alexcjohnson@Marc-Andre-Rivet