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

Skip slow tests in pre-commit hook #3132

Merged
merged 4 commits into from
Dec 8, 2023
Merged

Skip slow tests in pre-commit hook #3132

merged 4 commits into from
Dec 8, 2023

Conversation

jdangerx
Copy link
Member

@jdangerx jdangerx commented Dec 7, 2023

The property-based test that makes sure our XBRL deduplication logic is working is quite slow. This test alone adds ~15-20s onto our full unit test suite - which is quite a lot! Additionally, it sometimes errors because test case generation time is erratic.

So:

  • I increased the deadline on the test case generation
  • I tagged all 3 tests that took >1s to execute with the pytest.mark.slow mark. This allows us to skip them in the pre-commit hook, but we will still run them in CI as well as when you just run pytest test/unit or pytest test/unit/the-file.py. This cuts the unit test timing on my machine from ~60s to ~10s.

I think it's reasonable to say that we should remove the property-based test and revisit when the performance isn't such an issue, or write a data generation strategy that makes a bunch of records and turns them into DataFrames.

@jdangerx jdangerx changed the base branch from main to dev December 7, 2023 16:51
Copy link
Member

@zaneselvans zaneselvans left a comment

Choose a reason for hiding this comment

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

This is a nice ergonomics improvement, thanks!

Unrelated to your code changes, I note that most (but not all!) of the version changes in the pinned dependencies are downgrades from what's on dev which seems a bit odd. They include psycopg2 and libcurl and I wonder if this is maybe some other project downstream identifying and avoiding issues that we've also run into?

@jdangerx jdangerx marked this pull request as ready for review December 7, 2023 23:00
@zaneselvans
Copy link
Member

Side-note, based on #3133 investigations, it looks like the PostgreSQL weirdness is somehow related to OpenSSL 3.2.0 weirdness, which is causing trouble for other projects as well, and that's why we've got all the downgrades here.

Copy link
Member

@bendnorman bendnorman left a comment

Choose a reason for hiding this comment

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

This is great, thank you!

Side note: I kicked off a full build from this branch to make sure the version downgrades solve #3133 which they do! So, hopefully, dev passes tonight 🤞

@bendnorman bendnorman linked an issue Dec 8, 2023 that may be closed by this pull request
Copy link

codecov bot commented Dec 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0407adb) 92.6% compared to head (1be965a) 92.6%.
Report is 2 commits behind head on dev.

Additional details and impacted files
@@          Coverage Diff          @@
##             dev   #3132   +/-   ##
=====================================
  Coverage   92.6%   92.6%           
=====================================
  Files        134     134           
  Lines      12573   12576    +3     
=====================================
+ Hits       11645   11648    +3     
  Misses       928     928           

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

@zaneselvans zaneselvans merged commit 2e37b3a into dev Dec 8, 2023
15 checks passed
@zaneselvans zaneselvans deleted the slow-hypothesis-test branch December 8, 2023 04:59
@bendnorman bendnorman mentioned this pull request Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Speed up unit tests
3 participants