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

Add semantic model test to test_contracts_graph_parsed.py #8654

Merged
merged 3 commits into from
Oct 9, 2023

Conversation

QMalcolm
Copy link
Contributor

@QMalcolm QMalcolm commented Sep 15, 2023

resolves #8139

Problem

The tests in test_contracts_graph_parsed.py are meant to ensure that we can go from objects to dictionaries and back without any changes. There are two problems this PR aims to solve

  1. these tests are brittle, and are tedious to fix (we want a simpler style of test)
  2. we lacked a test of this style for semantic models

Solution

We added a test for semantic models symmetry. The test depends on a new package hypothesis. The @given parameterizes a test, with it generating the arguements it has following strategies. The main strategies we use is builds this takes in a callable passes any sub strategies for named arguements, and will try to infer any other arguments if the callable is typed. I found that even though the test was run many many times, some of the SemanticModel properties weren't being changed. For instance dimensions, entities, and measures were always empty lists. Because of this I defined sub strategies for some attributes of SemanticModels.

Checklist

  • I have read the contributing guide and understand what's expected of me
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX

The tests in `test_contracts_graph_parsed.py` are meant to ensure
that we can go from objects to dictionaries and back without any
changes. We've had a desire to simplify these tests. Most tests in
this file have three to four fixtures, this test only has one. What
a test of this format ensures is that parsing a SemanticModel from
a dictionary doesn't add/drop any keys from the dictionary and that
when going back to the dictionary no keys are dropped. This style of
test will still break whenever the semantic model (or sub objects)
change. However now when that happens, only one fixture will have to
be updated (whereas previously we had to update 3-4 fixtures).
@QMalcolm QMalcolm added the Skip Changelog Skips GHA to check for changelog file label Sep 15, 2023
@cla-bot cla-bot bot added the cla:yes label Sep 15, 2023
@QMalcolm QMalcolm marked this pull request as ready for review September 15, 2023 23:54
@QMalcolm QMalcolm requested a review from a team as a code owner September 15, 2023 23:54
@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (3f5ebe8) 86.60% compared to head (94494a4) 86.50%.
Report is 33 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8654      +/-   ##
==========================================
- Coverage   86.60%   86.50%   -0.10%     
==========================================
  Files         175      176       +1     
  Lines       25638    25856     +218     
==========================================
+ Hits        22204    22368     +164     
- Misses       3434     3488      +54     
Flag Coverage Δ
integration 83.25% <ø> (-0.21%) ⬇️
unit 65.07% <ø> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 39 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Hypothesis is a python package for doing property testing. The `@given`
parameterizes a test, with it generating the arguements it has following
`strategies`. The main strategies we use is `builds` this takes in a callable
passes any sub strategies for named arguements, and will try to infer any
other arguments if the callable is typed. I found that even though the
test was run many many times, some of the `SemanticModel` properties
weren't being changed. For instance `dimensions`, `entities`, and `measures`
were always empty lists. Because of this I defined sub strategies for
some attributes of `SemanticModel`s.
Copy link
Member

@emmyoop emmyoop left a comment

Choose a reason for hiding this comment

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

Can you add some info to the existing README (that's empty...) about when we generally want to use hypothesis?

@QMalcolm QMalcolm requested a review from emmyoop October 6, 2023 19:55
@martynydbt martynydbt merged commit 70b2e15 into main Oct 9, 2023
50 checks passed
@martynydbt martynydbt deleted the qmalcolm--8139-test-semantic-model-graph-contract branch October 9, 2023 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes Skip Changelog Skips GHA to check for changelog file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-2842] Add SemanticModel tests to test_contracts_graph_parsed
4 participants