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

feat: add backfill one off scripts #734

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

joseph-sentry
Copy link
Contributor

these are one off python scripts to be run directly that will modify the database

the first backfills the daily test rollups starting on a given day, and ending on a given date, starting for a given repo (it will backfill in repo id order)

the second backfills the TestFlagBridge objects for all repos with the option to focus on a given repo, specified by repoid

these are one off python scripts to be run directly that will
modify the database

the first backfills the daily test rollups starting on a given day,
and ending on a given date, starting for a given repo (it will
backfill in repo id order)

the second backfills the TestFlagBridge objects for all repos with the
option to focus on a given repo, specified by repoid
@codecov-notifications
Copy link

codecov-notifications bot commented Sep 23, 2024

Codecov Report

Attention: Patch coverage is 12.32227% with 185 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
one_off_scripts/backfill_daily_test_rollups.py 0.00% 101 Missing ⚠️
..._scripts/tests/test_backfill_daily_test_rollups.py 14.28% 36 Missing ⚠️
one_off_scripts/backfill_test_flag_bridges.py 25.00% 21 Missing ⚠️
...f_scripts/tests/test_backfill_test_flag_bridges.py 33.33% 18 Missing ⚠️
one_off_script.py 0.00% 9 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (5601e39) and HEAD (2d834d0). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (5601e39) HEAD (2d834d0)
unit 2 1
integration 2 1

Impacted file tree graph

@@             Coverage Diff             @@
##             main     #734       +/-   ##
===========================================
- Coverage   98.09%   25.14%   -72.95%     
===========================================
  Files         432      438        +6     
  Lines       36332    36543      +211     
===========================================
- Hits        35640     9189    -26451     
- Misses        692    27354    +26662     
Flag Coverage Δ
integration 25.14% <12.32%> (-72.95%) ⬇️
unit 25.14% <12.32%> (-72.95%) ⬇️

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

Components Coverage Δ
NonTestCode 34.50% <7.74%> (-61.53%) ⬇️
OutsideTasks 26.91% <12.32%> (-71.22%) ⬇️
Files with missing lines Coverage Δ
one_off_scripts/__init__.py 100.00% <100.00%> (ø)
one_off_script.py 0.00% <0.00%> (ø)
...f_scripts/tests/test_backfill_test_flag_bridges.py 33.33% <33.33%> (ø)
one_off_scripts/backfill_test_flag_bridges.py 25.00% <25.00%> (ø)
..._scripts/tests/test_backfill_daily_test_rollups.py 14.28% <14.28%> (ø)
one_off_scripts/backfill_daily_test_rollups.py 0.00% <0.00%> (ø)

... and 401 files with indirect coverage changes

@codecov-qa
Copy link

codecov-qa bot commented Sep 23, 2024

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
1 1 0 0
View the top 1 failed tests by shortest run time
 one_off_scripts.tests.test_backfill_daily_test_rollups
Stack Traces | 0s run time
No failure message available

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

Copy link

codecov-public-qa bot commented Sep 23, 2024

Test Failures Detected: Due to failing tests, we cannot provide coverage reports at this time.

❌ Failed Test Results:

Completed 1 tests with 1 failed, 0 passed and 0 skipped.

View the full list of failed tests

pytest

  • Class name:
    Test name: one_off_scripts.tests.test_backfill_daily_test_rollups

    No failure message available

Copy link

codecov bot commented Sep 23, 2024

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
1 1 0 0
View the top 1 failed tests by shortest run time
 one_off_scripts.tests.test_backfill_daily_test_rollups
Stack Traces | 0s run time
No failure message available

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

Comment on lines +5 to +6
os.environ.setdefault("DJANGO_SETTINGS_MODULE", "django_scaffold.settings")
django.setup()
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason this is duplicated from the main script? does this actually run twice when you import one of the child scripts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These scripts are meant to be run separately from the actual application running on a pod so we'd invoke it directly using python one_off_script.py and that means we have to setup django separately

Comment on lines 120 to 121
f"Updating test instances {start_repoid} {start_date} {end_date}",
extra=dict(start_repoid=start_repoid, start_date=start_date, end_date=end_date),
Copy link
Contributor

Choose a reason for hiding this comment

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

as you have all these fields in the extra as well, I would remove them from the log message, it only makes it harder to search for all the individual logs.

/ (obj.pass_count + obj.fail_count),
)

rollup.save()
Copy link
Contributor

Choose a reason for hiding this comment

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

django has a bulk_create method as well, though I recommend to always define a reasonable chunk size.

rollup.save()


def run_impl(
Copy link
Contributor

Choose a reason for hiding this comment

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

please give this a more descriptive name :-)

f"Updating test instances {start_repoid} {start_date} {end_date}",
extra=dict(start_repoid=start_repoid, start_date=start_date, end_date=end_date),
)
test_analytics_repos = get_test_analytics_repos(start_repoid)
Copy link
Contributor

Choose a reason for hiding this comment

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

you should probably also iterate over these in chunks, because you are also logging the whole list.
unless you know for certain there is only a small number of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah there aren't many Repository objects that would be returned from this function

Comment on lines 34 to 36
).select_related("upload")[:1][0]

if first_test_instance is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

I’m not 100% sure how this works with using the slicing syntax to define a limit, whether you actually get a list of appropriate length with None filled in?

I think this might just throw because you are indexing out of bounds if no such object exists?

Comment on lines 42 to 43
new_bridge = TestFlagBridge(test=test, flag=flags[flag_name])
new_bridge.save()
Copy link
Contributor

Choose a reason for hiding this comment

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

again, bulk_create should be more appropriate here.

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