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: pre risky migration flaky test changes #524

Merged
merged 6 commits into from
Jul 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions database/models/reports.py
Original file line number Diff line number Diff line change
Expand Up @@ -322,3 +322,28 @@ class TestResultReportTotals(CodecovBaseModel, MixinBaseClass):
skipped = Column(types.Integer)
failed = Column(types.Integer)
error = Column(types.String(100), nullable=True)


class ReducedError(CodecovBaseModel, MixinBaseClass):
__tablename__ = "reports_reducederror"
message = Column(types.Text)


class Flake(CodecovBaseModel, MixinBaseClass):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the idea to then replace these w/ their django counterparts after you run the risky migration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, these will remain, these changes just don't depend on the risky migrations to run

Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm is there a reason we don't want to use the django models instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's possible the Flake model is not needed, but the ReducedError model is needed because we will be writing the reduced_error_id in the test results processor in future changes

Copy link
Contributor

@adrian-codecov adrian-codecov Jun 26, 2024

Choose a reason for hiding this comment

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

After talking, we'll defer the use of django models as it poses some difficulties with testing atm. Happy to approve once you have addressed @michelletran-codecov's feedback 👌

__tablename__ = "reports_flake"
repoid = Column(types.Integer, ForeignKey("repos.repoid"))
repository = relationship("Repository", backref=backref("flakes"))

testid = Column(types.Text, ForeignKey("reports_test.id"))
test = relationship(Test, backref=backref("flakes"))

reduced_error_id = Column(
types.Integer, ForeignKey("reports_reducederror.id"), nullable=True
)
reduced_error = relationship(ReducedError, backref=backref("flakes"))

recent_passes_count = Column(types.Integer)
count = Column(types.Integer)
fail_count = Column(types.Integer)
start_date = Column(types.DateTime)
end_date = Column(types.DateTime, nullable=True)
5 changes: 3 additions & 2 deletions requirements.in
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
https://github.com/codecov/shared/archive/390e62c4858ee41ffd035e5d7fe77c71a179d76a.tar.gz#egg=shared
https://github.com/codecov/opentelem-python/archive/refs/tags/v0.0.4a1.tar.gz#egg=codecovopentelem
https://github.com/codecov/test-results-parser/archive/5515e960d5d38881036e9127f86320efca649f13.tar.gz#egg=test-results-parser
https://github.com/codecov/test-results-parser/archive/1507de2241601d678e514c08b38426e48bb6d47d.tar.gz#egg=test-results-parser
boto3>=1.34
celery>=5.3.6
click
Expand Down Expand Up @@ -38,11 +38,12 @@ SQLAlchemy-Utils
SQLAlchemy
statsd
stripe>=9.6.0
time-machine
https://github.com/codecov/timestring/archive/d37ceacc5954dff3b5bd2f887936a98a668dda42.tar.gz#egg=timestring
urllib3>=1.26.18
vcrpy
opentelemetry-instrumentation-celery>=0.45b0
opentelemetry-sdk>=1.24.0
django-admin
google-cloud-pubsub
openai
openai
5 changes: 4 additions & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,7 @@ python-dateutil==2.9.0.post0
# django-postgres-extra
# faker
# freezegun
# time-machine
# timeconvert
python-json-logger==0.1.11
# via -r requirements.in
Expand Down Expand Up @@ -403,10 +404,12 @@ statsd==3.3.0
# shared
stripe==9.6.0
# via -r requirements.in
test-results-parser @ https://github.com/codecov/test-results-parser/archive/5515e960d5d38881036e9127f86320efca649f13.tar.gz
test-results-parser @ https://github.com/codecov/test-results-parser/archive/1507de2241601d678e514c08b38426e48bb6d47d.tar.gz
# via -r requirements.in
text-unidecode==1.3
# via faker
time-machine==2.14.1
# via -r requirements.in
timeconvert==3.0.13
# via excel-base
timestring @ https://github.com/codecov/timestring/archive/d37ceacc5954dff3b5bd2f887936a98a668dda42.tar.gz
Expand Down
40 changes: 5 additions & 35 deletions services/test_results.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from shared.yaml import UserYaml
from sqlalchemy import desc

from database.enums import FlakeSymptomType, ReportType
from database.enums import ReportType
from database.models import Commit, CommitReport, RepositoryFlag, TestInstance, Upload
from services.report import BaseReportService
from services.repository import (
Expand Down Expand Up @@ -119,19 +119,13 @@ class TestResultsNotificationFailure:
test_id: str


@dataclass
class TestResultsNotificationFlake:
flake_type: list[FlakeSymptomType]
is_new_flake: bool


@dataclass
class TestResultsNotificationPayload:
failed: int
passed: int
skipped: int
failures: List[TestResultsNotificationFailure]
flaky_tests: dict[str, TestResultsNotificationFlake] | None = None
flaky_tests: set[str] | None = None


class TestResultsNotifier:
Expand Down Expand Up @@ -210,32 +204,11 @@ def build_message(self, payload: TestResultsNotificationPayload) -> str:

completed = payload.failed + payload.passed + payload.skipped
if payload.flaky_tests:
new_flakes = 0
existing_flakes = 0

for failure in payload.failures:
flake = payload.flaky_tests.get(failure.test_id, None)
if flake is not None:
if flake.is_new_flake:
new_flakes += 1
else:
existing_flakes += 1
existing_flake_section = (
f"{existing_flakes} known flaky" if existing_flakes else ""
)
comma_section = ", " if new_flakes and existing_flakes else ""
new_flake_section = (
f"{new_flakes} newly detected flaky" if new_flakes else ""
)
flake_section = (
f"({existing_flake_section}{comma_section}{new_flake_section})"
if (existing_flakes or new_flakes)
else ""
)
num = len(payload.flaky_tests)
flake_section = f"({num} known flakes hit)" if (num) else ""

results = [
f"Completed {completed} tests with **`{payload.failed} failed`**{flake_section}, {payload.passed} passed and {payload.skipped} skipped.",
f"- Total :snowflake:**{len(payload.flaky_tests)} flaky tests.**",
]
message += results
else:
Expand All @@ -246,10 +219,7 @@ def build_message(self, payload: TestResultsNotificationPayload) -> str:
flake_dict = defaultdict(list)
for fail in payload.failures:
flake = None
if payload.flaky_tests is not None:
flake = payload.flaky_tests.get(fail.test_id, None)

if flake is not None:
if payload.flaky_tests is not None and fail.test_id in payload.flaky_tests:
flake_dict[fail.testsuite].append(fail)
else:
fail_dict[fail.testsuite].append(fail)
Expand Down
12 changes: 3 additions & 9 deletions services/tests/test_test_results.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,9 @@
import pytest
from shared.torngit.exceptions import TorngitClientError

from database.enums import FlakeSymptomType
from database.models.core import Commit
from services.test_results import (
TestResultsNotificationFailure,
TestResultsNotificationFlake,
TestResultsNotificationPayload,
TestResultsNotifier,
generate_flags_hash,
Expand Down Expand Up @@ -130,20 +128,16 @@ def test_build_message_with_flake(tn):
fail = TestResultsNotificationFailure(
"hello world", "testsuite", "testname", [], test_id
)
flake = TestResultsNotificationFlake(
[FlakeSymptomType.FAILED_IN_DEFAULT_BRANCH],
True,
)
payload = TestResultsNotificationPayload(1, 2, 3, [fail], {test_id: flake})

payload = TestResultsNotificationPayload(1, 2, 3, [fail], {test_id})
res = tn.build_message(payload)

assert (
res
== """**Test Failures Detected**: Due to failing tests, we cannot provide coverage reports at this time.

### :x: Failed Test Results:
Completed 6 tests with **`1 failed`**(1 newly detected flaky), 2 passed and 3 skipped.
- Total :snowflake:**1 flaky tests.**
Completed 6 tests with **`1 failed`**(1 known flakes hit), 2 passed and 3 skipped.
<details><summary>View the full list of flaky tests</summary>

## testsuite
Expand Down
Loading
Loading