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

Remove tags & retries tests since they're not applicable to most projects #1146

Closed
wants to merge 3 commits into from

Conversation

BasPH
Copy link
Contributor

@BasPH BasPH commented Apr 10, 2023

Removing the pytest cases testing for DAG tags & retries, following discussion in https://astronomer.slack.com/archives/CL44PA4DB/p1680001384869019.

Most customers don't have tags & retries configured on all DAGs so I don't think we should enforce that via tests.

Description

Describe the purpose of this pull request.

🎟 Issue(s)

🧪 Functional Testing

Tested change by doing a fresh astro dev init and running astro dev pytest successfully.

📸 Screenshots

Add screenshots to illustrate the validity of these changes.

📋 Checklist

  • Rebased from the main (or release if patching) branch (before testing)
  • Ran make test before taking out of draft
  • Ran make lint before taking out of draft
  • Added/updated applicable tests
  • Tested against Astro-API (if necessary).
  • Tested against Houston-API and Astronomer (if necessary).
  • Communicated to/tagged owners of respective clients potentially impacted by these changes.
  • Updated any related documentation

@dylanbstorey
Copy link
Contributor

I'm also in favor of this as its an opinionated style piece that is non obvious and not "generally" applicable. Maybe just comment them out and allow people to see what the tests look like and do what they will vs full deletion though ?

@BasPH
Copy link
Contributor Author

BasPH commented Apr 11, 2023

I'd rather explain that in docs than having commented code, for example under https://docs.astronomer.io/astro/test-and-troubleshoot-locally#run-tests-with-pytest or https://docs.astronomer.io/astro/cli/astro-dev-pytest.

@dylanbstorey
Copy link
Contributor

Should we open a matching issue with our docs page ?

@sunkickr
Copy link
Contributor

sunkickr commented May 3, 2023

@dylanbstorey @BasPH has a docs issues been opened yet?

@dylanbstorey
Copy link
Contributor

A general issue was opened on this subject, not 100% sure where its at though.

@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (89aeb92) 87.75% compared to head (18bd1f5) 87.75%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1146   +/-   ##
=======================================
  Coverage   87.75%   87.75%           
=======================================
  Files         108      108           
  Lines       11199    11199           
=======================================
  Hits         9828     9828           
  Misses        809      809           
  Partials      562      562           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kushalmalani
Copy link
Contributor

@BasPH Is this good to merge?

@erdos2n
Copy link
Contributor

erdos2n commented Sep 22, 2023

I've written a similar issue where we don't delete the pytest but we can skip them using a variable. The PyTest is helpful and from a reliability perspective helps customers with DAG integrity, but it should be configurable.

Issue #1398

@erdos2n
Copy link
Contributor

erdos2n commented Sep 28, 2023

@BasPH can we close this PR? This looks to be handled by PR #1404

@sunkickr sunkickr closed this Oct 10, 2023
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.

5 participants