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 Flake models and refactor flake detection #346

Merged
merged 14 commits into from
Apr 9, 2024

Conversation

joseph-sentry
Copy link
Contributor

@joseph-sentry joseph-sentry commented Mar 25, 2024

Refactors flake detection to use FlakeDetectionEngine and symptom detectors

@codecov-notifications
Copy link

codecov-notifications bot commented Mar 25, 2024

Codecov Report

Attention: Patch coverage is 97.40260% with 4 lines in your changes are missing coverage. Please review.

✅ All tests successful. No failed tests found ☺️

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #346   +/-   ##
=======================================
  Coverage   97.64%   97.64%           
=======================================
  Files         393      393           
  Lines       32811    32874   +63     
=======================================
+ Hits        32038    32100   +62     
- Misses        773      774    +1     
Flag Coverage Δ
integration 97.64% <97.40%> (+<0.01%) ⬆️
latest-uploader-overall 97.64% <97.40%> (+<0.01%) ⬆️
unit 97.64% <97.40%> (+<0.01%) ⬆️

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

Components Coverage Δ
NonTestCode 95.14% <96.11%> (+<0.01%) ⬆️
OutsideTasks 97.79% <97.40%> (+<0.01%) ⬆️
Files Coverage Δ
database/enums.py 100.00% <100.00%> (ø)
services/tests/test_flake_detector.py 100.00% <100.00%> (ø)
services/flake_detection.py 96.63% <95.95%> (-0.11%) ⬇️

@codecov-qa
Copy link

codecov-qa bot commented Mar 25, 2024

Codecov Report

Attention: Patch coverage is 97.40260% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 97.64%. Comparing base (579ca4a) to head (0132b4b).

✅ All tests successful. No failed tests found ☺️

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #346   +/-   ##
=======================================
  Coverage   97.64%   97.64%           
=======================================
  Files         393      393           
  Lines       32811    32874   +63     
=======================================
+ Hits        32038    32100   +62     
- Misses        773      774    +1     
Flag Coverage Δ
integration 97.64% <97.40%> (+<0.01%) ⬆️
latest-uploader-overall 97.64% <97.40%> (+<0.01%) ⬆️
unit 97.64% <97.40%> (+<0.01%) ⬆️

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

Components Coverage Δ
NonTestCode 95.14% <96.11%> (+<0.01%) ⬆️
OutsideTasks 97.79% <97.40%> (+<0.01%) ⬆️
Files Coverage Δ
database/enums.py 100.00% <100.00%> (ø)
services/tests/test_flake_detector.py 100.00% <100.00%> (ø)
services/flake_detection.py 96.63% <95.95%> (-0.11%) ⬇️

Copy link

codecov-public-qa bot commented Mar 25, 2024

Codecov Report

Attention: Patch coverage is 97.40260% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 97.64%. Comparing base (579ca4a) to head (0132b4b).

✅ All tests successful. No failed tests found ☺️

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #346   +/-   ##
=======================================
  Coverage   97.64%   97.64%           
=======================================
  Files         393      393           
  Lines       32811    32874   +63     
=======================================
+ Hits        32038    32100   +62     
- Misses        773      774    +1     
Flag Coverage Δ
integration 97.64% <97.40%> (+<0.01%) ⬆️
latest-uploader-overall 97.64% <97.40%> (+<0.01%) ⬆️
unit 97.64% <97.40%> (+<0.01%) ⬆️

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

Components Coverage Δ
NonTestCode 95.14% <96.11%> (+<0.01%) ⬆️
OutsideTasks 97.79% <97.40%> (+<0.01%) ⬆️
Files Coverage Δ
database/enums.py 100.00% <100.00%> (ø)
services/tests/test_flake_detector.py 100.00% <100.00%> (ø)
services/flake_detection.py 96.63% <95.95%> (-0.11%) ⬇️

Copy link

codecov bot commented Mar 25, 2024

Codecov Report

Attention: Patch coverage is 97.40260% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 97.65%. Comparing base (579ca4a) to head (0132b4b).

✅ All tests successful. No failed tests found ☺️

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #346   +/-   ##
=======================================
  Coverage   97.65%   97.65%           
=======================================
  Files         424      424           
  Lines       33511    33574   +63     
=======================================
+ Hits        32726    32788   +62     
- Misses        785      786    +1     
Flag Coverage Δ
integration 97.64% <97.40%> (+<0.01%) ⬆️
latest-uploader-overall 97.64% <97.40%> (+<0.01%) ⬆️
unit 97.64% <97.40%> (+<0.01%) ⬆️

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

Components Coverage Δ
NonTestCode 95.13% <96.11%> (+<0.01%) ⬆️
OutsideTasks 97.79% <97.40%> (+<0.01%) ⬆️
Files Coverage Δ
database/enums.py 100.00% <100.00%> (ø)
services/tests/test_flake_detector.py 100.00% <100.00%> (ø)
services/flake_detection.py 96.63% <95.95%> (-0.11%) ⬇️

This change has been scanned for critical changes. Learn more

@@ -77,6 +83,7 @@ def create_test_instance(dbsession, test_id, upload_id, outcome, failure_message
)
dbsession.add(ti)
dbsession.flush()
return ti
Copy link
Contributor

@thomasrockhu-codecov thomasrockhu-codecov Apr 3, 2024

Choose a reason for hiding this comment

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

liveyourlife

self.is_curr_flake = False
self.flake_dict.clear()
self.commit_to_outcome.clear()
@cached_property
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remember that having a class-wide cached property is not a good idea cause the cache is shared among all instances of the class... I think that was the gotcha.

I do think that each of these classes should return a single FlakeSymtomType so this seems fine. Probably it's what you intended, but just stating it again

Copy link
Contributor

Choose a reason for hiding this comment

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

That being said, I don't think we gain anything by having this cached.... in all your implementations it's just a constant being returned. Maybe I'd remove this cached_property, but nit in any case.

FAILED_IN_DEFAULT_BRANCH = "failed_in_default_branch"
CONSECUTIVE_DIFF_OUTCOMES = "consecutive_diff_outcomes"
UNRELATED_MATCHING_FAILURES = "unrelated_matching_failures"
classes that implement this interface are expected to:
Copy link
Contributor

Choose a reason for hiding this comment

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

[very nit] I think this can be broken up into docstrings in the abstract methods directly... I feel that this could very easily become obsolete and people forget to update it.

@joseph-sentry joseph-sentry merged commit 2311041 into main Apr 9, 2024
26 of 30 checks passed
@joseph-sentry joseph-sentry deleted the joseph/flake-models branch April 9, 2024 13:51
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.

3 participants