-
Notifications
You must be signed in to change notification settings - Fork 28
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
fix: add unique constraint on test model #357
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #357 +/- ##
=======================================
Coverage 96.07% 96.07%
=======================================
Files 631 632 +1
Lines 16398 16408 +10
=======================================
+ Hits 15754 15764 +10
Misses 644 644
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov Report
@@ Coverage Diff @@
## main #357 +/- ##
=======================================
Coverage 96.07% 96.07%
=======================================
Files 631 632 +1
Lines 16398 16408 +10
=======================================
+ Hits 15754 15764 +10
Misses 644 644
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #357 +/- ##
=======================================
+ Coverage 95.74 95.75 +0.01
=======================================
Files 746 747 +1
Lines 16914 16924 +10
=======================================
+ Hits 16194 16204 +10
Misses 720 720
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: joseph-sentry <[email protected]>
2f98bf8
to
33d9a0e
Compare
|
||
|
||
class Migration(migrations.Migration): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you pls ad the SQL code for this migration here? 🙇
# users will use flags to distinguish the same test being run | ||
# in a different environment | ||
# for example: the same test being run on windows vs. mac | ||
flags_hash = models.TextField() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh interesting, is this the same flag as a codecov flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a hash of the flags associated with the uploads that map to this test.
The idea is that users will use flags with their uploads to differentiate between the same test being run in a different environment (this used to be the env field but now it's the flags_hash field).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I'd expect this field to be nullable too right?
reports/models.py
Outdated
@@ -224,6 +224,7 @@ class Test(models.Model): | |||
# in this case is because we want to be able to compute/predict | |||
# the primary key of a Test object ourselves in the processor | |||
# so we can easily do concurrent writes to the database | |||
# this is a hash of the repoid, name, testsuite and env |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, so the id itself will account for the env now 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i should rename it to flags_hash
in the comment here as well but yes, because a test is unique on the flags_hash as well, we will include it in the hash
Signed-off-by: joseph-sentry <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Purpose/Motivation
The outcome field should be an enum stored as a string.
The env field should renamed to flags_hash for clarity.
The uniqueconstraint was added to ensure that Test objects are unique according to repo, name, testsuite and flags_hash.
What does this PR do?