feat: add models and task for failed test ingestion #197

joseph-sentry · 2023-12-01T22:47:50Z

Depends on: codecov/test-results-parser#4

codecov-qa · 2023-12-01T22:52:29Z

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (292c555) 98.13% compared to head (ea5afbf) 98.12%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #197      +/-   ##
==========================================
- Coverage   98.13%   98.12%   -0.01%     
==========================================
  Files         370      373       +3     
  Lines       29996    30480     +484     
==========================================
+ Hits        29437    29909     +472     
- Misses        559      571      +12

Flag	Coverage Δ
integration	`98.12% <97.32%> (-0.01%)`	⬇️
latest-uploader-overall	`98.12% <97.32%> (-0.01%)`	⬇️
unit	`98.12% <97.32%> (-0.01%)`	⬇️

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

Components	Coverage Δ
NonTestCode	`96.18% <94.06%> (-0.04%)`	⬇️
OutsideTasks	`97.91% <88.88%> (-0.03%)`	⬇️

Files	Coverage Δ
database/models/reports.py	`99.35% <100.00%> (+0.14%)`	⬆️
...sks/tests/unit/test_test_results_processor_task.py	`100.00% <100.00%> (ø)`
tasks/tests/unit/test_upload_task.py	`99.49% <100.00%> (+0.01%)`	⬆️
tasks/upload.py	`97.93% <91.66%> (-0.57%)`	⬇️
tasks/test_results_processor.py	`97.56% <97.56%> (ø)`
services/test_results.py	`81.39% <81.39%> (ø)`

... and 1 file with indirect coverage changes

codecov · 2023-12-01T22:52:29Z

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (292c555) 98.11% compared to head (ea5afbf) 98.12%.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #197      +/-   ##
==========================================
+ Coverage   98.11%   98.12%   +0.01%     
==========================================
  Files         401      373      -28     
  Lines       30697    30480     -217     
==========================================
- Hits        30117    29909     -208     
+ Misses        580      571       -9

Flag	Coverage Δ
integration	`98.12% <97.32%> (-0.01%)`	⬇️
latest-uploader-overall	`98.12% <97.32%> (-0.01%)`	⬇️
onlysomelabels	`?`
unit	`98.12% <97.32%> (-0.01%)`	⬇️

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

Components	Coverage Δ
NonTestCode	`96.18% <94.06%> (+0.06%)`	⬆️
OutsideTasks	`97.91% <88.88%> (-0.03%)`	⬇️

Files	Coverage Δ
database/models/reports.py	`99.35% <100.00%> (+0.14%)`	⬆️
...sks/tests/unit/test_test_results_processor_task.py	`100.00% <100.00%> (ø)`
tasks/tests/unit/test_upload_task.py	`99.49% <100.00%> (+0.01%)`	⬆️
tasks/upload.py Critical	`97.93% <91.66%> (-0.59%)`	⬇️
tasks/test_results_processor.py	`97.56% <97.56%> (ø)`
services/test_results.py	`81.39% <81.39%> (ø)`

... and 74 files with indirect coverage changes

This change has been scanned for critical changes. Learn more

codecov-staging · 2024-01-03T16:45:13Z

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

@@            Coverage Diff             @@
##             main     #197      +/-   ##
==========================================
- Coverage   98.13%   98.12%   -0.01%     
==========================================
  Files         370      373       +3     
  Lines       29996    30480     +484     
==========================================
+ Hits        29437    29909     +472     
- Misses        559      571      +12

Flag	Coverage Δ
integration	`98.12% <97.32%> (-0.01%)`	⬇️
latest-uploader-overall	`98.12% <97.32%> (-0.01%)`	⬇️
unit	`98.12% <97.32%> (-0.01%)`	⬇️

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

Components	Coverage Δ
NonTestCode	`96.18% <94.06%> (-0.04%)`	⬇️
OutsideTasks	`97.91% <88.88%> (-0.03%)`	⬇️

Files	Coverage Δ
database/models/reports.py	`99.35% <100.00%> (+0.14%)`	⬆️
...sks/tests/unit/test_test_results_processor_task.py	`100.00% <100.00%> (ø)`
tasks/tests/unit/test_upload_task.py	`99.49% <100.00%> (+0.01%)`	⬆️
tasks/upload.py	`97.93% <91.66%> (-0.57%)`	⬇️
tasks/test_results_processor.py	`97.56% <97.56%> (ø)`
services/test_results.py	`81.39% <81.39%> (ø)`

... and 1 file with indirect coverage changes

codecov-public-qa · 2024-01-03T16:45:33Z

Codecov Report

Merging #197 (ea5afbf) into main (292c555) will decrease coverage by 0.01%.
The diff coverage is 97.32%.

@@            Coverage Diff             @@
##             main     #197      +/-   ##
==========================================
- Coverage   98.13%   98.12%   -0.01%     
==========================================
  Files         370      373       +3     
  Lines       29996    30480     +484     
==========================================
+ Hits        29437    29909     +472     
- Misses        559      571      +12

Flag	Coverage Δ
integration	`98.12% <97.32%> (-0.01%)`	⬇️
latest-uploader-overall	`98.12% <97.32%> (-0.01%)`	⬇️
unit	`98.12% <97.32%> (-0.01%)`	⬇️

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

Components	Coverage Δ
NonTestCode	`96.18% <94.06%> (-0.04%)`	⬇️
OutsideTasks	`97.91% <88.88%> (-0.03%)`	⬇️

Files	Coverage Δ
database/models/reports.py	`99.35% <100.00%> (+0.14%)`	⬆️
...sks/tests/unit/test_test_results_processor_task.py	`100.00% <100.00%> (ø)`
tasks/tests/unit/test_upload_task.py	`99.49% <100.00%> (+0.01%)`	⬆️
tasks/upload.py	`97.93% <91.66%> (-0.57%)`	⬇️
tasks/test_results_processor.py	`97.56% <97.56%> (ø)`
services/test_results.py	`81.39% <81.39%> (ø)`

... and 1 file with indirect coverage changes

trent-codecov · 2024-01-04T15:15:32Z

tasks/test_results_processor.py

+    ):
+        for testrun in testrun_list:
+            test = (
+                db_session.query(Test)


This is an n+1 query. Query for the list of tests outside the loop. Then check for existence in the loop.

tasks/test_results_processor.py

trent-codecov · 2024-01-04T15:18:00Z

tasks/test_results_processor.py

+                elif b"pytest" in first_line:
+                    testrun_list = parse_pytest_reportlog(file_content)
+                else:
+                    testrun_list = parse_vitest_json(file_content)


Should we maybe match something for vitest and then "else" raise a not supported error?

This might get weird otherwise.

giovanni-guidini · 2024-01-04T20:40:15Z

tasks/test_results_processor.py

+                parsed_testruns: List[Testrun] = self.process_individual_arg(
+                    upload_obj, upload_obj.report.commit.repository
+                )
+            except Exception:


It's fine to leave a catch all exception for starters so we avoid crashing the task while we battle test it. But please be explicit about it, as having a catch-all exception is not good practice.

Also the warning message won't be helpful when debugging. Make sure to include the stack trace so we know what to fix (in case there's anything)

giovanni-guidini · 2024-01-04T20:51:00Z

tasks/test_results_processor.py

+                return {"successful": False}
+
+            # concat existing and new test information
+            testrun_list += parsed_testruns


Looking at the upload task it seems that you decided to go with a group, that is parallel in nature (meaning we might be processing multiple uploads at the same time for the same commit).

This indicates that it's ok to be saving data concurrently for the same commit. I wonder then if it's necessary to accumulate the parsing results in testrun_list prior to saving them. I think this list might become possibly big (we know of users that have +100K tests, for example), and this seems like it could use a fairly large amount of data.

I could save the instances as soon as we parse them, but for tests I think it's better to avoid doing it on every iteration since we will be getting the list of all tests for that repository

giovanni-guidini · 2024-01-04T20:51:32Z

tasks/test_results_processor.py

+        test_set = set()
+        for test in repo_tests:
+            test_set.add(f"{test.testsuite}::{test.name}")
+        for testrun in testrun_list:


I wonder if we can leverage https://docs.sqlalchemy.org/en/13/orm/session_api.html#sqlalchemy.orm.session.Session.merge session.merge here to offload this logic to the database...

I have a feeling it'd be more efficient, if possible.

EDIT probably we can't cause it looks at the primary key of the instance and ... well we don't have an instance yet 😅

i think we could upsert https://docs.sqlalchemy.org/en/14/dialects/postgresql.html#insert-on-conflict-upsert

giovanni-guidini · 2024-01-04T20:53:11Z

tasks/test_results_processor.py

+            # TODO: improve report matching capabilities
+            # use file extensions?
+            # maybe do the matching in the testing result parser lib?
+            first_line = bytes("".join(first_line.decode("utf-8").split()), "utf-8")


[nit] report matching should be in its own function

giovanni-guidini · 2024-01-04T20:55:05Z

tasks/test_results_processor.py

+                elif first_line.startswith(b'"{"numTotalTestSuites":'):
+                    testrun_list = parse_vitest_json(file_content)
+            except Exception as e:
+                log.warning(f"Error parsing: {file_content.decode()}")


Please use a static message an add extra args to the logger. That will make it easier to search for logs and it's the norm for worker logs

giovanni-guidini · 2024-01-04T20:57:05Z

tasks/tests/unit/test_test_results_processor_task.py

+                {
+                    "duration_seconds": 0.001,
+                    "name": "api.temp.calculator.test_calculator::test_add",
+                    "outcome": "Outcome.Pass",


Where is this "Outcome.Pass" defined? In the model the field is an Integer, but I can't find the Enum for it....

it's defined in the test-results-parser library

Can we import the type definition into the python code? nice... works like an enum or does it think it's just a string?

EDIT ah I see the docs https://pyo3.rs/main/class
very nice very nice

matt-codecov

first off: awesome work on the feature, i think the code is designed pretty well and i think we'll be able to build on it well for flaky test detection, etc

just a callout: we don't have a pattern in place or all the missing pieces we need to avoid loading everything into memory here, so beware that this implementation is at risk of the same performance problems coverage is until we address that

archive_service.read_file() reads into memory, we'd need to save to disk
json.loads() doesn't stream, we'd need another json parser
(above is enough to avoid loading more than one results file into memory at once)
(if we are worried individual results files will be enormous we also need the following)
zlib appears to have a streaming decompression util https://docs.python.org/3/library/zlib.html#zlib.decompressobj
a streaming base64 decoder
a rework of the rust code to take a bytes stream abstraction instead of a concrete Vec<u8> or &[u8]

matt-codecov · 2024-01-10T01:44:18Z

tasks/test_results_processor.py

+                log.error(
+                    "File did not match any parser format",
+                    extra=dict(
+                        file_content=file_bytes.read().decode(),


this will blow up, maybe only print the first 300 chars or something

matt-codecov · 2024-01-10T01:44:45Z

tasks/test_results_processor.py

+                log.error(
+                    "Error parsing test result file",
+                    extra=dict(
+                        file_content=file_content.decode(),


this will blow up, maybe limit to 300 chars or something

matt-codecov · 2024-01-10T02:15:53Z

tasks/test_results_processor.py

+        test_set = set()
+        for test in repo_tests:
+            test_set.add(f"{test.testsuite}::{test.name}")
+        for testrun in testrun_list:


i think we could upsert https://docs.sqlalchemy.org/en/14/dialects/postgresql.html#insert-on-conflict-upsert

matt-codecov · 2024-01-11T03:52:18Z

database/models/reports.py

+    testsuite = Column(types.String(256), nullable=False)
+
+    __table_args__ = (
+        UniqueConstraint(


I think since API runs migrations we'll need a change there to actually create this constraint if that hasn't been done

if id_ contains all of these things and it's a proper PK then this constraint may not be necessary

codecov/codecov-api#357 because id_ is just a sha256 hash of all of those

matt-codecov

un-accepting until we talk about whether this perf change is feasible quickly, worth blocking for, or should be deferred

matt-codecov · 2024-01-17T06:20:29Z

database/models/reports.py

+
+class TestInstance(CodecovBaseModel, MixinBaseClass):
+    __tablename__ = "reports_testinstance"
+    test_id = Column(types.Integer, ForeignKey("reports_test.id"))


hold up, if we change this to a hash of the test suite + test name we might be able to unlock performance improvements

the reason we can't do the db work in the processor task is because we need to synchronize to get/create records of the Test model because we need to put its id on the TestInstance model. if the ID could be computed from the test suite + test name, then we don't need to synchronize with other tasks to create our TestInstance records

as for creating the corresponding Test records, we could do INSERT ... ON CONFLICT DO NOTHING or INSERT ... WHERE NOT EXISTS (...) to attempt inserts in a fire-and-forget way and the database will handle synchronizing internally

we would have to hash the env as well but I agree that this may get rid of the issue where we want to use a returning clause

matt-codecov · 2024-01-17T06:29:37Z

tasks/upload.py

+        if processor_task_group:
+            checkpoint_data = None
+            if checkpoints:
+                checkpoints.log(UploadFlow.INITIAL_PROCESSING_COMPLETE)


i think we need to use different flows for coverage, bundles, and test results. we have totally different expectations/norms for how long each takes, and putting them together in one pool makes the metrics useless

Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>

…_list Going to move the save report part to the finisher to avoid concurrency issues

This commit moves the writes in the test result finisher back to the test result processor task. The first step is to change the primary key of the Test object to a text field. This is so we can use a computed primary key for Test objects. By computing the test ID using the generate_test_id function, we avoid having to use a returning clause from the insert on conflict do nothing for Test objects. The timestamp field on the TestInstance object has also been removed as its replaced by using the upload created_at to determine the order of TestInstance objects to be showed in the PR comment. Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>

Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>

we don't want to fail the entire upload parsing because one test result file failed to be parsed. Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>

joseph-sentry force-pushed the joseph/failed-test-ingestion branch 2 times, most recently from a351e98 to 99fe006 Compare December 8, 2023 22:27

joseph-sentry changed the title ~~feat: add models for failed test ingestion~~ feat: add models and task for failed test ingestion Dec 8, 2023

joseph-sentry force-pushed the joseph/failed-test-ingestion branch 2 times, most recently from 82cc9a5 to 76ea63d Compare December 14, 2023 17:36

joseph-sentry force-pushed the joseph/failed-test-ingestion branch 2 times, most recently from a68c001 to fb700f2 Compare January 3, 2024 14:22

joseph-sentry mentioned this pull request Jan 3, 2024

Add test result ingestion in the API codecov/codecov-api#218

Merged

joseph-sentry force-pushed the joseph/failed-test-ingestion branch 2 times, most recently from 037f374 to eb250a6 Compare January 3, 2024 20:48

joseph-sentry requested review from giovanni-guidini, matt-codecov and adrian-codecov January 3, 2024 21:59

trent-codecov reviewed Jan 4, 2024

View reviewed changes

joseph-sentry force-pushed the joseph/failed-test-ingestion branch from 769d55a to b493ab3 Compare January 4, 2024 20:47

giovanni-guidini reviewed Jan 4, 2024

View reviewed changes

joseph-sentry requested a review from giovanni-guidini January 5, 2024 21:30

joseph-sentry force-pushed the joseph/failed-test-ingestion branch 2 times, most recently from f296188 to 37f85b1 Compare January 9, 2024 21:31

matt-codecov requested changes Jan 10, 2024

View reviewed changes

joseph-sentry force-pushed the joseph/failed-test-ingestion branch 2 times, most recently from d522306 to 28fd629 Compare January 12, 2024 18:54

joseph-sentry mentioned this pull request Jan 16, 2024

Add test results finisher #234

Merged

matt-codecov approved these changes Jan 17, 2024

View reviewed changes

matt-codecov requested changes Jan 17, 2024

View reviewed changes

matt-codecov reviewed Jan 17, 2024

View reviewed changes

joseph-sentry added 26 commits January 25, 2024 10:58

chore: fix CI failure

6b5e4a8

chore: rename testrun duration to duration_seconds

5c44ed2

chore: update deps

6764a69

chore: update deps

86ffb44

tests: update test_result upload tests

e4920c7

tests: add vitest and pytest reportlog tests for test_result_processor

9075411

chore: try another fix?

ce9105f

chore: last try to fix

45c61b2

address feedback and improve tests

6025381

fix previous unfinished commit

255f84e

improve error handling in test_results_processor

4ecef71

update test_results_parser version

cd33944

chore: remove useless log

7f0e6f7

feat: add support for ingesting failure messages

344ced5

chore: update testinstace table name

63b7350

chore: update testinstance backref names

358d1c3

Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>

address feedback

7d06d8c

fix: handle concurrent test object updates

dc385c5

fix: fix concurrent test insert handling

f5c96e7

chore: update test results parser version

87b12aa

fix: dont save report in processor and add failure_message to testrun…

90fe540

…_list Going to move the save report part to the finisher to avoid concurrency issues

fix: remove reference to timestamp

83ddf33

Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>

fix: add comments and remove unnecessary inheritance

0218ce2

Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>

fix(test_results): change env to flags_hash

7b0cdcc

Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>

Change outcome field on TestInstance to string

e37e0e6

Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>

joseph-sentry force-pushed the joseph/failed-test-ingestion branch from 0deb7c9 to e37e0e6 Compare January 25, 2024 17:41

fix(test_results): handle parse_single_file failures

ea5afbf

we don't want to fail the entire upload parsing because one test result file failed to be parsed. Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>

joseph-sentry merged commit d686ff9 into main Jan 25, 2024
16 of 31 checks passed

joseph-sentry deleted the joseph/failed-test-ingestion branch January 25, 2024 19:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add models and task for failed test ingestion #197

feat: add models and task for failed test ingestion #197

joseph-sentry commented Dec 1, 2023 •

edited

Loading

codecov-qa bot commented Dec 1, 2023 •

edited

Loading

codecov bot commented Dec 1, 2023 •

edited

Loading

codecov-staging bot commented Jan 3, 2024 •

edited

Loading

codecov-public-qa bot commented Jan 3, 2024 •

edited

Loading

trent-codecov Jan 4, 2024

trent-codecov Jan 4, 2024

giovanni-guidini Jan 4, 2024

giovanni-guidini Jan 4, 2024

joseph-sentry Jan 4, 2024

giovanni-guidini Jan 4, 2024 •

edited

Loading

matt-codecov Jan 10, 2024

giovanni-guidini Jan 4, 2024

giovanni-guidini Jan 4, 2024

giovanni-guidini Jan 4, 2024

joseph-sentry Jan 4, 2024

giovanni-guidini Jan 5, 2024 •

edited

Loading

matt-codecov left a comment

matt-codecov Jan 10, 2024

matt-codecov Jan 10, 2024

matt-codecov Jan 10, 2024

matt-codecov Jan 11, 2024

matt-codecov Jan 24, 2024

joseph-sentry Jan 24, 2024

matt-codecov left a comment

matt-codecov Jan 17, 2024

joseph-sentry Jan 17, 2024

matt-codecov Jan 17, 2024

feat: add models and task for failed test ingestion #197

feat: add models and task for failed test ingestion #197

Conversation

joseph-sentry commented Dec 1, 2023 • edited Loading

codecov-qa bot commented Dec 1, 2023 • edited Loading

Codecov Report

codecov bot commented Dec 1, 2023 • edited Loading

Codecov Report

codecov-staging bot commented Jan 3, 2024 • edited Loading

Codecov Report

codecov-public-qa bot commented Jan 3, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giovanni-guidini Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giovanni-guidini Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

matt-codecov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matt-codecov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joseph-sentry commented Dec 1, 2023 •

edited

Loading

codecov-qa bot commented Dec 1, 2023 •

edited

Loading

codecov bot commented Dec 1, 2023 •

edited

Loading

codecov-staging bot commented Jan 3, 2024 •

edited

Loading

codecov-public-qa bot commented Jan 3, 2024 •

edited

Loading

giovanni-guidini Jan 4, 2024 •

edited

Loading

giovanni-guidini Jan 5, 2024 •

edited

Loading