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

Add an e2e-ish test for the Upload flow #613

Merged
merged 1 commit into from
Aug 14, 2024
Merged

Conversation

Swatinem
Copy link
Contributor

Instead of relying on overly mocked unit tests, this rather adds a mostly end-to-end test. It Pushes uploads to Storage/Redis, and then invokes the Upload task, running it to completion. After completion of the whole upload flow, it runs assertions on the final report after all processing is complete.

@Swatinem Swatinem requested a review from a team August 13, 2024 14:43
@Swatinem Swatinem self-assigned this Aug 13, 2024
@codecov-notifications
Copy link

codecov-notifications bot commented Aug 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #613      +/-   ##
==========================================
+ Coverage   97.59%   97.66%   +0.06%     
==========================================
  Files         431      433       +2     
  Lines       36152    36245      +93     
==========================================
+ Hits        35283    35399     +116     
+ Misses        869      846      -23     
Flag Coverage Δ
integration 97.66% <100.00%> (+0.06%) ⬆️
latest-uploader-overall 97.66% <100.00%> (+0.06%) ⬆️
unit 97.66% <100.00%> (+0.06%) ⬆️

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

Components Coverage Δ
NonTestCode 94.95% <100.00%> (+0.16%) ⬆️
OutsideTasks 97.89% <ø> (+0.07%) ⬆️
Files Coverage Δ
tasks/tests/integration/test_upload_e2e.py 100.00% <100.00%> (ø)
tasks/tests/utils.py 100.00% <100.00%> (ø)

... and 8 files with indirect coverage changes

Copy link

codecov-public-qa bot commented Aug 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.66%. Comparing base (1fedc79) to head (0bc6f48).

✅ All tests successful. No failed tests found.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #613      +/-   ##
==========================================
+ Coverage   97.59%   97.66%   +0.06%     
==========================================
  Files         431      433       +2     
  Lines       36152    36245      +93     
==========================================
+ Hits        35283    35399     +116     
+ Misses        869      846      -23     
Flag Coverage Δ
integration 97.66% <100.00%> (+0.06%) ⬆️
latest-uploader-overall 97.66% <100.00%> (+0.06%) ⬆️
unit 97.66% <100.00%> (+0.06%) ⬆️

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

Components Coverage Δ
NonTestCode 94.95% <100.00%> (+0.16%) ⬆️
OutsideTasks 97.89% <ø> (+0.07%) ⬆️
Files Coverage Δ
tasks/tests/integration/test_upload_e2e.py 100.00% <100.00%> (ø)
tasks/tests/utils.py 100.00% <100.00%> (ø)

... and 8 files with indirect coverage changes

Copy link

codecov bot commented Aug 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.07%. Comparing base (1fedc79) to head (0bc6f48).

Changes have been made to critical files, which contain lines commonly executed in production. Learn more

✅ All tests successful. No failed tests found.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #613      +/-   ##
==========================================
+ Coverage   97.63%   98.07%   +0.43%     
==========================================
  Files         466      409      -57     
  Lines       37358    29294    -8064     
==========================================
- Hits        36475    28729    -7746     
+ Misses        883      565     -318     
Flag Coverage Δ
integration ?
latest-uploader-overall ?
unit ?

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

Components Coverage Δ
NonTestCode 96.17% <ø> (+1.30%) ⬆️
OutsideTasks 97.92% <ø> (+0.07%) ⬆️

see 269 files with indirect coverage changes

This change has been scanned for critical changes. Learn more

@codecov-qa
Copy link

codecov-qa bot commented Aug 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.66%. Comparing base (1fedc79) to head (0bc6f48).

✅ All tests successful. No failed tests found.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #613      +/-   ##
==========================================
+ Coverage   97.59%   97.66%   +0.06%     
==========================================
  Files         431      433       +2     
  Lines       36152    36245      +93     
==========================================
+ Hits        35283    35399     +116     
+ Misses        869      846      -23     
Flag Coverage Δ
integration 97.66% <100.00%> (+0.06%) ⬆️
latest-uploader-overall 97.66% <100.00%> (+0.06%) ⬆️
unit 97.66% <100.00%> (+0.06%) ⬆️

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

Components Coverage Δ
NonTestCode 94.95% <100.00%> (+0.16%) ⬆️
OutsideTasks 97.89% <ø> (+0.07%) ⬆️
Files Coverage Δ
tasks/tests/integration/test_upload_e2e.py 100.00% <100.00%> (ø)
tasks/tests/utils.py 100.00% <100.00%> (ø)

... and 8 files with indirect coverage changes

Copy link
Contributor

@giovanni-guidini giovanni-guidini left a comment

Choose a reason for hiding this comment

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

Hoping the comments are useful

tasks/tests/integration/test_upload_e2e.py Outdated Show resolved Hide resolved
tasks/tests/integration/test_upload_e2e.py Outdated Show resolved Hide resolved
tasks/tests/integration/test_upload_e2e.py Outdated Show resolved Hide resolved
tasks/tests/integration/test_upload_e2e.py Outdated Show resolved Hide resolved
@Swatinem Swatinem force-pushed the swatinem/e2e-upload branch from 4fa6c4d to d1b643e Compare August 14, 2024 08:43
Instead of relying on overly mocked unit tests, this rather adds a mostly end-to-end test.
It Pushes uploads to Storage/Redis, and then invokes the `Upload` task, running it to completion.
After completion of the whole upload flow, it runs assertions on the final report after all processing is complete.
@Swatinem Swatinem force-pushed the swatinem/e2e-upload branch from d1b643e to 0bc6f48 Compare August 14, 2024 09:04
Copy link
Contributor

@giovanni-guidini giovanni-guidini left a comment

Choose a reason for hiding this comment

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

LGTM, I think we should merge this verison now.

I'd like to also add some considerations for "future work"

What happened to the notifications? There was a task scheduled and the requests likely failed... we might want to verify that in the future?

There are some inner details on the reports we have had issues with in the past (particularly around the sessions), it would be something else that we might add more asserts into.

The carryforward logic would benefit from having an e2e test for it as well I believe

@Swatinem Swatinem added this pull request to the merge queue Aug 14, 2024
@Swatinem
Copy link
Contributor Author

What happened to the notifications? There was a task scheduled and the requests likely failed... we might want to verify that in the future?

Yes, I believe I saw some log output mentioning that those failed.

There are some inner details on the reports we have had issues with in the past (particularly around the sessions), it would be something else that we might add more asserts into.
The carryforward logic would benefit from having an e2e test for it as well I believe

Totally agree that it makes sense to add more parts to this test, or introduce more similar tests.

Merged via the queue into main with commit 1ebcc5a Aug 14, 2024
25 of 26 checks passed
@Swatinem Swatinem deleted the swatinem/e2e-upload branch August 14, 2024 09:43
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