-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat: new ta tasks #976
feat: new ta tasks #976
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: tasks/upload.py
Did you find this useful? React with a 👍 or 👎 |
This PR includes changes to |
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
📢 Thoughts on this report? Let us know! |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #976 +/- ##
==========================================
- Coverage 97.79% 97.74% -0.05%
==========================================
Files 447 451 +4
Lines 36175 36653 +478
==========================================
+ Hits 35376 35828 +452
- Misses 799 825 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
❌ 1 Tests Failed:
View the top 1 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
❌ 1 Tests Failed:
View the top 1 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
✅ All tests successful. No failed tests were found. 📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
tasks/upload.py
Outdated
arguments_list=list(chunk), | ||
) | ||
for chunk in itertools.batched(argument_list, CHUNK_SIZE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we still want to run these in batches, or rather one upload per task?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one upload per task seems reasonable now that we aren't writing to the db in the processor
|
||
def test_test_analytics(dbsession, mocker, celery_app): | ||
url = "literally/whatever" | ||
storage_service = get_appropriate_storage_service(None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe you want to use the mock storage provider for this?
mocker.patch.object(TAProcessorTask, "app", celery_app) | ||
mocker.patch.object(TAFinisherTask, "app", celery_app) | ||
|
||
hello = celery_app.register_task(ProcessFlakesTask()) | ||
_ = celery_app.tasks[hello.name] | ||
goodbye = celery_app.register_task(CacheTestRollupsTask()) | ||
_ = celery_app.tasks[goodbye.name] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have never seen this pattern, what does it do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without this, when the finisher would try to call those tasks, they weren't in the mocked celery app, so what i was trying to do here is add them to the mocked celery app
i replaced this with some code that is hopefully more clear
user-agent: | ||
- Default | ||
method: GET | ||
uri: https://api.github.com/repos/ThiagoCodecov/example-python/commits/abf6d4df662c47e32460020ab14abf9303581429 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you rather mock away whatever call does this request, instead of relying on vcr?
tasks/ta_finisher.py
Outdated
for upload in uploads: | ||
repo_flag_ids = get_repo_flag_ids(db_session, repoid, upload.flag_names) | ||
if upload.state == "processed": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this loop has a couple of problems:
- you are querying all uploads from the DB, but only ever run the code on
processed
ones - you only append to
tests_to_write
and friends, but never clear those across uploads save_tests
and friends runs for all the uploads, together with never clearingtests_to_write
above means that you insert the same tests over and over again depending on how many total uploads you have- you unconditionally set
state = "finished"
for all the downloads, also ones that already have that state - the intermediate msgpack file is never cleared.
fd1401b
to
1296951
Compare
@Swatinem sorry i got confused and rebased and force pushed but i really just added 5 new commits on top of the existing ones and didn't modify any of the existing ones |
This PR includes changes to |
report__commit__repository__repoid=repo_id, | ||
report__commit__commitid=commit_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe commit_id
already uniquely identifies the report, so no need for an additional repository
join.
This PR includes changes to |
one_off_scripts/__init__.py
Outdated
@@ -1,6 +0,0 @@ | |||
import os |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m 👍🏻 on removing these if they won’t ever be used again, but its probably best to do that in a separate PR.
0bfc4ca
to
469958e
Compare
This PR includes changes to |
this commit essentially does 3 things: - creates the new ta_processor and ta_finisher tasks - the difference between these tasks and the old ones is that these ones use upload states differently - these ones also use the TA storage module to persist data to BQ - updates the version of the test results parser being used - we've gone from parsing individual JUnit XML files to parsing the entire raw upload at once - creates the ta_storage module - the ta_storage module serves as an abstraction for persisting data to both PG and BQ
469958e
to
4159b65
Compare
This PR includes changes to |
this PR creates new ta processor and finisher tasks and uses them behind a feature flag in the upload task for a smooth rollout