Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce assert_component_equal for tests #195

Merged
merged 16 commits into from
Dec 20, 2023

Conversation

antonymilne
Copy link
Contributor

@antonymilne antonymilne commented Dec 6, 2023

Description

I made a new testing utility function assert_component_equal as part of rewriting of tests in #70 but didn't advertise it widely before. I've just tweaked how it works so I'm happy with it now. I'll do a learning session on this on Weds 20th December to explain all.

Just in terms of reviewing, there's not really anything to check here because most of the commits are just tidying up my implementation of assert_component_equal.

The things that are of interest are:

Basic usage

When we compare the output of a build method to a Dash component tree, previously we were doing something like this:

result = json.loads(json.dumps(x, cls=plotly.utils.PlotlyJSONEncoder))
expected = json.loads(json.dumps(y, cls=plotly.utils.PlotlyJSONEncoder))
assert result == expected

Now we do:

assert_component_equal(x, y)

For example:

from dash import html
assert_component_equal(html.Div([html.P("blah")]), html.Div([html.P("blah")]))

So it's just a nicer way of writing the same check. Think of it a bit like pandas.testing.assert_frame_equal.

Advanced usage: keys_to_strip

assert_component_equal also has a keys_to_strip argument that sets properties that will be ignored in the component comparison. Sometimes it's useful to set key_to_strip={"id"} since our ids are randomly generated (they're model manager ids) and we don't care about actually comparing them.

The other use for this is in testing "high-level containers" vs. "low-level contents". This is used a lot in the navigation tests and demonstrated with a toy example in https://github.com/mckinsey/vizro/blob/tests/assert-components-equal/vizro-core/tests/tests_utils/demo_asserts.py.

In general any test where we have keys_to_strip={"children"} would be followed by a test on result.children using STRIP_ALL.

The idea is:

  1. test_X_build tests stuff that's only in X
  2. test_Y_build tests stuff that's only in Y
  3. test_Y_build also tests that Y.build() is correctly calling X.build() but not the details of what X.build() returns

The typical way you'd do case 3 is by providing a mock X and then doing something like assert X.call_count == 4. The current solution is arguably slightly "wrong" in that some of the implementation of X.build is leaking into the test_Y_build, because we are checking against html.Div. This is fine by me though, because it means we test the interface a bit more strictly, and it's much clearer, easier and less error-prone than writing mocks.

Next steps

  • Convert all our tests that do json.loads(json.dump(...)) into assert_component_equal. Most of these will just be simple find and replace as per the basic usage
  • Write tests that are currently missing e.g. for Page.build using the "advanced usage"

Checklist

  • I have not referenced individuals, products or companies in any commits, directly or indirectly
  • I have not added data or restricted code in any commits, directly or indirectly
  • I have updated the docstring of any public function/class/model changed
  • I have added tests to cover my changes (if applicable)

Types of changes

  • Docs/refactoring (non-breaking change which improves codebase)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

Signed-off-by: Antony Milne <49395058+antonymilne@users.noreply.github.com>
@antonymilne antonymilne changed the title Tests/assert components equal Introduce assert_component_equal for tests Dec 19, 2023
Copy link
Contributor

@huong-li-nguyen huong-li-nguyen left a comment

Choose a reason for hiding this comment

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

Thanks for this super helpful PR description! 🚀 Everything is clear 👍

vizro-core/tests/tests_utils/demo_asserts.py Outdated Show resolved Hide resolved
Copy link
Contributor

@maxschulz-COL maxschulz-COL left a comment

Choose a reason for hiding this comment

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

Lgtm! Have one suggestion, but it's a really cool feature, and really saves a lot of confusion and headache :)

vizro-core/tests/tests_utils/asserts.py Outdated Show resolved Hide resolved
vizro-core/tests/tests_utils/demo_asserts.py Outdated Show resolved Hide resolved
Copy link
Contributor

@petar-qb petar-qb left a comment

Choose a reason for hiding this comment

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

I really like the "assert_component_equal".
P.S. Thanks for the awesome description! 🚀

vizro-core/tests/tests_utils/asserts.py Show resolved Hide resolved
@antonymilne antonymilne merged commit a30b50b into main Dec 20, 2023
38 checks passed
@antonymilne antonymilne deleted the tests/assert-components-equal branch December 20, 2023 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants