-
Notifications
You must be signed in to change notification settings - Fork 3k
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 job meta requests #7560
Fix job meta requests #7560
Conversation
@klakhov Could you please check tests? |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #7560 +/- ##
===========================================
- Coverage 83.53% 83.52% -0.01%
===========================================
Files 372 372
Lines 39661 39693 +32
Branches 3724 3729 +5
===========================================
+ Hits 33130 33153 +23
- Misses 6531 6540 +9
|
cvat-core/src/frames.ts
Outdated
@@ -379,14 +414,31 @@ Object.defineProperty(FrameData.prototype.data, 'implementation', { | |||
writable: false, | |||
}); | |||
|
|||
async function getJobMeta(jobID: number): Promise<FramesMetaData> { | |||
let meta: FramesMetaData; |
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 can't see how in general it solves the problem of two parallel requests.
It solves in current case when requests are sent one by one.
The first from findFrame
, the second from getFrame
.
But if they are sent in parallel, it will not work.
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.
Perhaps we need somehow memoize receiveJobMeta() function in order to return already existing promise for the second request (pending or resolved), but not rejected. If the first promise was rejected, probably it is better to resend the request.
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.
Done.
@@ -163,6 +163,7 @@ context('Manipulations with masks', { scrollBehavior: false }, () => { | |||
describe('Tests to make sure that empty masks cannot be created', () => { | |||
beforeEach(() => { | |||
cy.removeAnnotations(); | |||
cy.saveJob('PUT'); |
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 please clarify why it worked before?
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.
cy.saveJob('PATCH', 200, 'removeUnderlyingPixelsUndoRedo')
lines inside Empty masks are deleted using remove underlying pixels feature
case are depending on PATCH
annotations requests. But after removeAnnotations
, first update of the annotations is sent via PUT
request.
So before the fix first call of the cy.saveJob('PATCH'...
catched not the PATCH /annotations
, but PATCH data/meta
and worked well. After the fix PATCH data/meta
is not sent, so the test failed.
We can either:
- Look for
PUT
request in first call of thecy.saveJob(...
inside the test - Save job after each remove annotations, so all saves inside test are looking for
PATCH
requests.
Ive decided to go the second way.
Motivation and context
Pr fixes two problems:
How has this been tested?
Checklist
develop
branch[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.