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

Issue 840 - Fix proptypes & syntax highlighting, and improve tests #841

Merged
merged 14 commits into from
Oct 20, 2020

Conversation

Marc-Andre-Rivet
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet commented Oct 19, 2020

Closes #840

  • adds log checks for all table tests
  • standardizes tests/integration tests with tests/selenium tests using the newer fixture / pattern
  • removes python-3.6 CI job (to be removed on approval) and percy/dash-table-python-v0 (those two visual tests have been flaky since forever and never uncovered a bug anyway)

As part of improving the tests, found an additional regression breaking highlight.js after #814 -- this does not affect dcc as it uses 9.x and the usage there is correct.

@Marc-Andre-Rivet Marc-Andre-Rivet marked this pull request as ready for review October 19, 2020 23:01
@alexcjohnson
Copy link
Collaborator

Removing Py3.6 tests - interesting... I guess there's an argument that components only need to test with the latest Python, as they have no independent Python code, and in the main dash repo we create test components and test them against multiple Python versions, verifying that the generated code will all work across versions.

That said table has some Python helper functions for formatting, which it would be nice to test will work across versions. On the other hand how much longer are we even going to support Py2, which would be the main reason for such a test?

So in the end I'd say yeah, this is fine.

@@ -1,4 +1,4 @@
import highlightjs from 'highlight.js/lib/highlight';
import highlightjs from 'highlight.js/lib/core';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Want to upgrade highlight in DCC too while this is front of mind?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. And if tacking that on, might as well add Julia support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -85,6 +85,7 @@ def test_tbcp001_copy_paste_callback(test):

assert target.cell(1, 0).get_text() == "0"
assert target.cell(1, 1).get_text() == "MODIFIED"
assert len(test.get_log_errors()) == 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert len(test.get_log_errors()) == 0
assert test.get_log_errors() == []

That way if it fails you see what the error was

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point 9e80289

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

Looks great! Just one nonblocking suggestion for cleaner error handling. 💃

@Marc-Andre-Rivet
Copy link
Contributor Author

Marc-Andre-Rivet commented Oct 20, 2020

For #841 (comment) - the formatter tests are currently run with the unit tests, on py37 only. The py36 tests are a misnomer as all that job really did was (1) load two apps with table/graph and take a screenshot of them, (2) run some export tests. Tests in (1) don't really test anything, tests in (2) were moved with the other e2e tests.

@Marc-Andre-Rivet Marc-Andre-Rivet changed the title Issue 840 - Fix proptypes and improve tests Issue 840 - Fix proptypes & syntax highlighting, and improve tests Oct 20, 2020
@Marc-Andre-Rivet Marc-Andre-Rivet merged commit 570fbd9 into dev Oct 20, 2020
@Marc-Andre-Rivet Marc-Andre-Rivet deleted the 840-tests branch October 20, 2020 15:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad prop-type definition causing console errors
2 participants