Skip to content

feat: automatic insertion of primary key tests#203

Merged
virvirlopez merged 10 commits intodev/0.1.0from
feat/primary_key_tests
Apr 19, 2021
Merged

feat: automatic insertion of primary key tests#203
virvirlopez merged 10 commits intodev/0.1.0from
feat/primary_key_tests

Conversation

@virvirlopez
Copy link
Contributor

@virvirlopez virvirlopez commented Apr 7, 2021

Resolves #194

Description

Automatic insertion of the primary key tests into the primary key column.

If an SQL file has a configuration specifying the primary key, this new functionality will try (is try because if the tests did not pass it will not add them) to add the unique test and the null test (only if there are not implemented already).

How was the change tested

Ran it in local.

Checklist

(Ideally, all boxes are checked by the time we merged the PR, if you don't know how to do any of these don't hesitate to say so in the PR and we'll help you out.)

  • I formatted my PR name according to CONTRIBUTING.md
  • I added a news fragment to help populating the changelog as encouraged in CONTRIBUTING.md
  • I added "Closes #<issue_number>" in the "Issue Information" section (if no issue, feel free to tick thick the box anyway).

@codecov
Copy link

codecov bot commented Apr 7, 2021

Codecov Report

Merging #203 (bd1434e) into dev/0.1.0 (6900fc3) will decrease coverage by 2.38%.
The diff coverage is 54.05%.

Impacted file tree graph

@@              Coverage Diff              @@
##           dev/0.1.0     #203      +/-   ##
=============================================
- Coverage      84.29%   81.90%   -2.39%     
=============================================
  Files             18       18              
  Lines           1057     1205     +148     
=============================================
+ Hits             891      987      +96     
- Misses           166      218      +52     
Flag Coverage Δ
unittests 81.90% <54.05%> (-2.39%) ⬇️

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

Impacted Files Coverage Δ
dbt_sugar/core/task/audit.py 68.99% <0.00%> (+1.05%) ⬆️
dbt_sugar/core/task/doc.py 61.32% <36.84%> (-6.66%) ⬇️
dbt_sugar/core/task/base.py 85.00% <76.47%> (-0.98%) ⬇️
dbt_sugar/core/connectors/snowflake_connector.py 60.86% <0.00%> (-39.14%) ⬇️
dbt_sugar/core/main.py 74.10% <0.00%> (-0.32%) ⬇️
dbt_sugar/core/config/config.py 95.58% <0.00%> (+0.17%) ⬆️
dbt_sugar/core/flags.py 97.91% <0.00%> (+0.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6900fc3...bd1434e. Read the comment docs.

feat: adding tests
@virvirlopez virvirlopez force-pushed the feat/primary_key_tests branch from cceb33d to 868747c Compare April 7, 2021 20:31
@bastienboutonnet bastienboutonnet changed the title feat: automatic insertion of primary key tests. feat: automatic insertion of primary key tests Apr 8, 2021
@bastienboutonnet bastienboutonnet changed the base branch from main to dev/0.1.0 April 8, 2021 16:14
Copy link
Member

@bastienboutonnet bastienboutonnet left a comment

Choose a reason for hiding this comment

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

Really really cool @virvirlopez I've changed the base branch to dev/0.1.0 just to put those new features aside for a bit while we finalise and wait for potential issues with the current release in case we have to ship more patches.

I made some suggestions and questions for you.

Our coverage is taking a big hit. Do you think you could add tests on the add function? I know it's "just" a runner but the logic inside is actually not simple and there are a few boolean logic pieces that I would like to make sure we have covered. What do you think?

Other than that, I think it's gonna work especially if you address my concern around how we get the path of the corresponding schema.yml file (see my comment in the code)

Finally, if you could:

  • add "Resolves #ticket-numer in the PR description
  • write a news fragment for the changelog that would be amazing.

Oh one more: make sure you check the issue and see if any if what I say or link too makes sense #194

Virginia Lopez-Gil and others added 3 commits April 18, 2021 11:30
Copy link
Member

@bastienboutonnet bastienboutonnet left a comment

Choose a reason for hiding this comment

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

Looks good but can you make sure that you add a changelog entry please (instructions here: https://github.com/bitpicky/dbt-sugar/blob/main/CONTRIBUTING.md#shout-out-your-hardwork-in-the-changelog) this is a big feature so we want to make sure people know about it

Other than that the code change is fine with me.

@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Apr 19, 2021

Sourcery Code Quality Report

✅  Merging this PR will increase code quality in the affected files by 0.12%.

Quality metrics Before After Change
Complexity 4.96 ⭐ 4.88 ⭐ -0.08 👍
Method Length 70.82 🙂 69.72 🙂 -1.10 👍
Working memory 10.44 😞 10.48 😞 0.04 👎
Quality 67.82% 🙂 67.94% 🙂 0.12% 👍
Other metrics Before After Change
Lines 1924 2178 254
Changed files Quality Before Quality After Quality Change
dbt_sugar/core/task/audit.py 74.71% 🙂 74.73% 🙂 0.02% 👍
dbt_sugar/core/task/base.py 67.16% 🙂 67.22% 🙂 0.06% 👍
dbt_sugar/core/task/doc.py 64.73% 🙂 66.10% 🙂 1.37% 👍
tests/doc_task_test.py 65.29% 🙂 64.96% 🙂 -0.33% 👎

Here are some functions in these files that still need a tune-up:

File Function Complexity Length Working Memory Quality Recommendation
dbt_sugar/core/task/base.py BaseTask.find_model_schema_file 25 😞 138 😞 16 ⛔ 34.07% 😞 Refactor to reduce nesting. Try splitting into smaller methods. Extract out complex expressions
tests/doc_task_test.py test_order_schema_yml 0 ⭐ 417 ⛔ 34 ⛔ 34.92% 😞 Try splitting into smaller methods. Extract out complex expressions
dbt_sugar/core/task/base.py BaseTask.update_model_description_test_tags 25 😞 116 🙂 12 😞 42.09% 😞 Refactor to reduce nesting. Extract out complex expressions
tests/doc_task_test.py test_column_has_primary_key_tests 0 ⭐ 140 😞 26 ⛔ 49.25% 😞 Try splitting into smaller methods. Extract out complex expressions
tests/doc_task_test.py test_update_model_description_test_tags 0 ⭐ 121 😞 29 ⛔ 50.88% 🙂 Try splitting into smaller methods. Extract out complex expressions

Legend and Explanation

The emojis denote the absolute quality of the code:

  • ⭐ excellent
  • 🙂 good
  • 😞 poor
  • ⛔ very poor

The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request.


Please see our documentation here for details on how these metrics are calculated.

We are actively working on this report - lots more documentation and extra metrics to come!

Let us know what you think of it by mentioning @sourcery-ai in a comment.

Copy link
Member

@bastienboutonnet bastienboutonnet left a comment

Choose a reason for hiding this comment

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

Fantastic feature!

@virvirlopez virvirlopez merged commit e24455e into dev/0.1.0 Apr 19, 2021
@virvirlopez virvirlopez deleted the feat/primary_key_tests branch April 19, 2021 17:10
bastienboutonnet added a commit that referenced this pull request May 3, 2021
* feat: automatic insertion of primary key tests.

* feat: adding tests

feat: adding tests

* feat: cleaning documentation

* feat: change the way of getting the SQL file

* Add new tests

* 'Refactored by Sourcery' (#216)

Co-authored-by: Sourcery AI <>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* feat: adding changelog defintion primary key tests

* feat: adding changelog defintion primary key tests

* rename and rephrase news fragment (#219)

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Bastien Boutonnet <bastien.b1@gmail.com>
bastienboutonnet added a commit that referenced this pull request May 5, 2021
* feat: add describe table logic in snowflake connector (#212)

* implement `describe table` logic in SnowflakeConnector

* add flag for experimental feature in config

* update test expectations to have use_describe_snowflake False

* add use_describe_snowflake CLI arg parsing

* update cli consumption tests

* 'Refactored by Sourcery' (#213)

Co-authored-by: Sourcery AI <>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* add changelog entry

* lowercase column names

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* feat: automatic insertion of primary key tests (#203)

* feat: automatic insertion of primary key tests.

* feat: adding tests

feat: adding tests

* feat: cleaning documentation

* feat: change the way of getting the SQL file

* Add new tests

* 'Refactored by Sourcery' (#216)

Co-authored-by: Sourcery AI <>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* feat: adding changelog defintion primary key tests

* feat: adding changelog defintion primary key tests

* rename and rephrase news fragment (#219)

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Bastien Boutonnet <bastien.b1@gmail.com>

* refactor: add use_describe to base (#236)

* add `use_describe` to the base `get_columns_from_table` def

* 'Refactored by Sourcery' (#237)

Co-authored-by: Sourcery AI <>

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>

* feat: bootstrap task (#233)

* setup arg parser and skeleton of bootstrap task

* implement barebone all dbt models dict building

* iterate through models and get col list and schema path

* combine getting columns and finding descriptor

* update tests for bootstrap_task

* update tests for audit_task

* add model placeholders in bootstrap_task

add all models placeholders

* refactor dbt models info in a dataclass for cleaner type checking

* feat: bootstrap task (Sourcery refactored) (#234)

* 'Refactored by Sourcery'

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Co-authored-by: Sourcery AI <>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* fix typo in new fragment

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* feat: allow audit task to run bootstrap (#239)

* add bootstrap option to `audit` and call it if passed

* add news fragement

* feat: collect rich user input for tests with subprocess (#238)

* feat: collect rich user input for tests with subprocess

* fix: order of the combine list

Make nicer the logs

Secure variable

* add: feature.md

* [pre-commit.ci] pre-commit autoupdate (#242)

updates:
- [github.com/psf/black: 21.4b0 → 21.4b2](psf/black@21.4b0...21.4b2)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* add dbt sugar logo

* chore(deps): Bump sqlalchemy from 1.4.12 to 1.4.13 (#243)

Bumps [sqlalchemy](https://github.com/sqlalchemy/sqlalchemy) from 1.4.12 to 1.4.13.
- [Release notes](https://github.com/sqlalchemy/sqlalchemy/releases)
- [Changelog](https://github.com/sqlalchemy/sqlalchemy/blob/master/CHANGES)
- [Commits](https://github.com/sqlalchemy/sqlalchemy/commits)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore(deps-dev): Bump pytest from 6.2.3 to 6.2.4 (#244)

* feat: add redshift connector (#229)

* add RedshiftConnector engine creation module

* add news fragment

* move pytest-dictsdiff to dev requirements

* make read_profile use PostgresDbtProfilesModel pydantic validation class

* 'Refactored by Sourcery' (#231)

Co-authored-by: Sourcery AI <>

* Bump version: 0.0.0 → 0.1.0-a.0

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: virvirlopez <virbyte@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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