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

Refactor dependents tests using library usecase #1572

Merged
merged 14 commits into from
Sep 1, 2022

Conversation

Jyuart
Copy link
Contributor

@Jyuart Jyuart commented Aug 26, 2022

Fixes #1571

Utilize library use cases for dependents tests and refactor them to make them more readable.

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the master branch of the repository
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no
    visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@codecov-commenter
Copy link

Codecov Report

Merging #1572 (57135d5) into master (848a138) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1572   +/-   ##
=======================================
  Coverage   92.61%   92.61%           
=======================================
  Files         142      142           
  Lines        6353     6353           
=======================================
  Hits         5884     5884           
  Misses        469      469           
Flag Coverage Δ
pytest-backend 92.61% <100.00%> (ø)

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

Impacted Files Coverage Δ
db/dependents/dependents_utils.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Jyuart Jyuart marked this pull request as ready for review August 29, 2022 15:52
@Jyuart Jyuart requested a review from a team August 29, 2022 15:52
@Jyuart Jyuart added the pr-status: review A PR awaiting review label Aug 29, 2022
@Jyuart Jyuart requested a review from silentninja August 29, 2022 15:53
@silentninja silentninja self-assigned this Aug 30, 2022
# excluding the possibility of circulal reference
def test_circular_reference(engine, library_tables_oids):
with engine.begin() as conn:
conn.execute(text('ALTER TABLE "Publishers" ADD COLUMN "Top Publication" integer'))
Copy link
Contributor

@silentninja silentninja Aug 31, 2022

Choose a reason for hiding this comment

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

We should add a self-referential field to the dataset instead of creating it in the test case. I have added an issue for it here mathesar-foundation/mathesar-data-playground#5.

Don't worry about it making the change now, just add a to-do, so that we can refactor it later.

Copy link
Contributor

@silentninja silentninja Sep 1, 2022

Choose a reason for hiding this comment

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

Test cases are throwing up an error because it is not able to determine the schema, instead of using a raw SQL statement, a better option would be to replace with an equivalent function db.columns.operations.create.create_column

@silentninja silentninja enabled auto-merge August 31, 2022 19:26
Copy link
Contributor

@silentninja silentninja 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 to me, Thanks for the PR @Jyuart!

@silentninja silentninja merged commit c03a2b0 into master Sep 1, 2022
@silentninja silentninja deleted the refactor-dependents-tests branch September 1, 2022 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-status: review A PR awaiting review
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Refactor dependents tests to use library usecase
3 participants