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

ProxyService bugfix #1244

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

CawaAlreadyTaken
Copy link

No description provided.

@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

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

Comparison is base (7db9aa1) 69.45% compared to head (ef35e60) 69.38%.

Files Patch % Lines
cms/service/ProxyService.py 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1244      +/-   ##
==========================================
- Coverage   69.45%   69.38%   -0.07%     
==========================================
  Files         328      328              
  Lines       26196    26199       +3     
==========================================
- Hits        18195    18179      -16     
- Misses       8001     8020      +19     
Flag Coverage Δ
functionaltests 46.95% <0.00%> (-0.26%) ⬇️
unittests 56.68% <0.00%> (-0.04%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@wil93 wil93 left a comment

Choose a reason for hiding this comment

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

Thanks!

I left a couple comments. I wonder also: how to reproduce this bug?

submission.get_result().scored():
for operation in self.operations_for_score(submission):
self.enqueue(operation)
if task is not None:
Copy link
Member

Choose a reason for hiding this comment

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

It might be more readable to invert the check here (if task is None), returning early in that case. It would avoid having to indent the rest of the code.

Copy link
Author

Choose a reason for hiding this comment

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

You are right it would make it more readable. I think I fixed it and pushed, I am not sure it's the first time I open a pull request, sorry for any github-wise misbehaviour :D

for operation in self.operations_for_score(submission):
self.enqueue(operation)
else:
logger.warning("Dataset update for unexistent task %d.", task_id)
Copy link
Member

Choose a reason for hiding this comment

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

nit: unexistent -> non-existent

Copy link
Author

Choose a reason for hiding this comment

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

I just corrected each occurrence of it in the file

@wil93
Copy link
Member

wil93 commented Jan 18, 2024

Can you explain how this bug gets triggered? It could be useful to know it so we can write a test for it.

@CawaAlreadyTaken
Copy link
Author

While a contest is running, if the dataset for a task that does not belong to the running contest gets updated, the error is triggered.
Se system keeps working, but the python Traceback for the Exception is written in the logs, visible from the admin Dashboard.
I think task.contest_id should be used instead of task.contest.id.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants