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

Test charts #262

Draft
wants to merge 24 commits into
base: dev
Choose a base branch
from
Draft

Test charts #262

wants to merge 24 commits into from

Conversation

ruthenian8
Copy link
Member

Description

Test charts; add chart test data

Checklist

  • I have covered the code with tests
  • I have added comments to my code to help others understand it
  • I have updated the documentation to reflect the changes
  • I have performed a self-review of the changes

Copy link
Member

@RLKRo RLKRo left a comment

Choose a reason for hiding this comment

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

I feel like this should be a separate test from test_main -- it's supposed to be more of an integration test that checks the entire pipeline from data collection to statistics display.

Also, this new test should clear the clickhouse db and fill it with new data like test_tutorial.

@RLKRo
Copy link
Member

RLKRo commented Nov 1, 2023

I don't understand why the tests pass. According to logs test_charts runs before test_main:

tests/stats/test_charts.py .                                             [ 54%]
tests/stats/test_defaults.py ....                                        [ 57%]
tests/stats/test_instrumentation.py ..                                   [ 58%]
tests/stats/test_main.py ...                                             [ 60%]
tests/stats/test_tutorials.py ....                                       [ 62%]

And I don't see any calls to dff.stats.cli.import_dashboard inside of the test_charts. So shouldn't the dashboard config be empty at the time of testing?


On the same note, test_charts should call drop_superset_assets and import_dashboard (or main).
This may be the reason my local tests failed for chart_id=1:

Expected :[]
Actual   :[{'animals': 0.0}, {'root': 0.5}, {'animals': 1.0}, {'root': 0.5}]

Superset may have had a different configuration stored in the postgres db


Also, I noticed that when I introduced some changes to charts in #227 I didn't update the query_context parameter. That could be a reason why certain charts return empty data. I'm going to fix this in dev soon.


I also imagined that chart testing would be more meaningful than comparing hundreds of lines of raw data.

Something along the lines of

  1. Generate a pipeline with 100 flows and transitions between them.
  2. Run everything.
  3. Assert that there's n data points in the raw_data chart; assert the number of transitions shown in the transition_counts chart from flow_{i} to flow_{i+1} equals m; e.t.c.

Or in order to test for no context-leakage:
Generate a pipeline with a separate flow for each context_id; run it; assert that no chart shows a transition from one flow to another.

We could discuss which specific tests we want tomorrow.

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.

2 participants