fix: always modify line to account for special labels #113
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fixes https://l.codecov.dev/fYqS4O There was a situation where you only get `SpecialLabelsEnum` and so we were not calling `_possibly_modify_line_to_account_for_special_labels` (which is the thing that transforms the special label ENUM into a STR - so we can save it in the DB) Now this is giving an error, so we need to fix it. These changes do fix it. But why was that check there in the first place? I can only theorize reasons... it might be to save processing time (since changing every line of every file in the report is a lot of lines), and this condition simply slipped. OR it might indicate some config error in the ATS process - e.g. we should never just receive the global label. I suppose that we might have introduced this condition afterwards because when the code was written it was impossible the case of uploading without running tests. I think now it's at least possible. In any case I believe this fix should hold and we're not gonna have more problems because of it later on. But wait... the actual error is `TypeError: Object of type SpecialLabelsEnum is not JSON serializable`, why didn't you added that to `shared.utils.ReportEncoder.ReportEncoder`? You are correct about that, little grasshopper. My reasoning was essencially that `SpecialLabelsEnum` is defined in the worker level. We obviously have the logic to modify the labels in place there so they never trickle down to `shared`. I thought it more logical to handle `SpecialLabelsEnum` in the worker for all cases. I was opposed to moving it to `shared`, and I think just adding `Enum` in general to the `ReportEncoder` is too general.
giovanni-guidini
force-pushed
the
gio/fix-labels-enum-processing
branch
from
4db4db8
to
6b4712b
Compare
Codecov Report
@@ Coverage Diff @@
## main #113 +/- ##
=======================================
Coverage 98.43% 98.43%
=======================================
Files 343 343
Lines 26678 26691 +13
=======================================
+ Hits 26261 26274 +13
Misses 417 417
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov Report
@@ Coverage Diff @@
## main #113 +/- ##
=======================================
Coverage 98.43% 98.43%
=======================================
Files 343 343
Lines 26678 26691 +13
=======================================
+ Hits 26261 26274 +13
Misses 417 417
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov Report
Changes have been made to critical files, which contain lines commonly executed in production. Learn more @@ Coverage Diff @@
## main #113 +/- ##
=======================================
Coverage 98.47% 98.47%
=======================================
Files 364 364
Lines 26837 26850 +13
=======================================
+ Hits 26427 26440 +13
Misses 410 410
Flags with carried forward coverage won't be shown. Click here to find out more.
|
scott-codecov
approved these changes
Sep 25, 2023
|
This would be a situation where only a global change was made and nothing else? This would indicate that we received and processed a report in which only global code was annotated to have been executed. I think it means that no test was run, but some code was imported none the less... not exactly sure. Does this somehow indicate that every test needs to be rerun? Yeah, pretty much. If there are changes to the global lines of code all tests need to re-run. It depends on the diff in the HEAD tho (assuming this will be used as BASE) |
giovanni-guidini
added a commit
that referenced
this pull request
Sep 26, 2023
After merging #113 I decided to investigate a little more why we would receive nothing but SpecialLabels in a report. Turns out that if you generate the reports without `--cov-context=test` but do `coverage json --show-contexts` it does that. So having a report with only special labels might indicate that ATS is not properly configured, that is, the tests are not running as they should. Ideally we would have a better place to surface this to customers, but right now we don't. So the least we can do is add a warning. In this case, if we look at all the logs in the task, we will see this detail.
giovanni-guidini
added a commit
that referenced
this pull request
Sep 26, 2023
After merging #113 I decided to investigate a little more why we would receive nothing but SpecialLabels in a report. Turns out that if you generate the reports without `--cov-context=test` but do `coverage json --show-contexts` it does that. So having a report with only special labels might indicate that ATS is not properly configured, that is, the tests are not running as they should. Ideally we would have a better place to surface this to customers, but right now we don't. So the least we can do is add a warning. In this case, if we look at all the logs in the task, we will see this detail.
giovanni-guidini
added a commit
that referenced
this pull request
Sep 27, 2023
After merging #113 I decided to investigate a little more why we would receive nothing but SpecialLabels in a report. Turns out that if you generate the reports without `--cov-context=test` but do `coverage json --show-contexts` it does that. So having a report with only special labels might indicate that ATS is not properly configured, that is, the tests are not running as they should. Ideally we would have a better place to surface this to customers, but right now we don't. So the least we can do is add a warning. In this case, if we look at all the logs in the task, we will see this detail.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes https://l.codecov.dev/fYqS4O
There was a situation where you only get
SpecialLabelsEnumand so we were not calling_possibly_modify_line_to_account_for_special_labels(which is the thing that transformsthe special label ENUM into a STR - so we can save it in the DB)
Now this is giving an error, so we need to fix it. These changes do fix it.
But why was that check there in the first place?
I can only theorize reasons... it might be to save processing time (since changing every line of
every file in the report is a lot of lines), and this condition simply slipped. OR it might indicate
some config error in the ATS process - e.g. we should never just receive the global label.
I suppose that we might have introduced this condition afterwards because when the code was written
it was impossible the case of uploading without running tests. I think now it's at least possible.
In any case I believe this fix should hold and we're not gonna have more problems because of it later on.
But wait... the actual error is
TypeError: Object of type SpecialLabelsEnum is not JSON serializable,why didn't you added that to
shared.utils.ReportEncoder.ReportEncoder?You are correct about that, little grasshopper. My reasoning was essencially that
SpecialLabelsEnumis defined in the worker level. We obviously have the logic to modify the labels in place there so
they never trickle down to
shared.I thought it more logical to handle
SpecialLabelsEnumin the worker for all cases.I was opposed to moving it to
shared, and I think just addingEnumin general to theReportEncoderis too general.Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.