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.

Sign up for GitHub

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

Jump to bottom

Ensure pushStatus is properly running #7213

Merged
merged 2 commits into from
Feb 20, 2021
Merged

Ensure pushStatus is properly running #7213

merged 2 commits into from
Feb 20, 2021

Conversation

dplewis
Copy link
Member

@dplewis dplewis commented Feb 20, 2021 •
edited
Loading

New Pull Request Checklist

Issue Description

This fixes a flaky test.

PushController properly creates _PushStatus
Message:
Expected 'pending' to be 'running'.
Stack:
Error: Expected 'pending' to be 'running'.
at
at reconfigureServer.then.then.then.then.then.catch.then (/home/runner/work/parse-server/parse-server/spec/PushController.spec.js:618:48)
at process._tickCallback (internal/process/next_tick.js:68:7)

This issue is the status of a push doesn't get updated properly. Here are the push status.

  1. pending - initial state
  2. running - set Running
  3. running - still running, updated counts, numSent, numFailed etc.
  4. succeeded - final state

The test fails because the status in step 2 never gets updated to running and as a result step 3 is never set to running.

Approach

Ensure status is set to running if you are keeping track of pushes sent, failed etc. I don't know if there is a bigger issue here because it looks like the same is happening to the Job status flaky tests. This is fixing a potential server bug.

TODOs before merging

  • Add test cases
  • Add entry to changelog
  • Add changes to documentation (guides, repository pages, in-code descriptions)
  • Add security check
  • Add new Parse Error codes to Parse JS SDK
  • ...

@dplewis dplewis mentioned this pull request Feb 20, 2021
Closed
4 tasks
@codecov
Copy link

codecov bot commented Feb 20, 2021 •
edited
Loading

Codecov Report

Merging #7213 (a3d3931) into master (a649a9e) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7213      +/-   ##
==========================================
+ Coverage   94.00%   94.02%   +0.01%     
==========================================
  Files         172      172              
  Lines       12868    12869       +1     
==========================================
+ Hits        12097    12100       +3     
+ Misses        771      769       -2
Impacted Files Coverage Δ
src/StatusHandler.js 98.56% <100.00%> (+0.01%) ⬆️
src/ParseServerRESTController.js 98.50% <0.00%> (+1.49%) ⬆️
src/batch.js 93.10% <0.00%> (+1.72%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a649a9e...a3d3931. Read the comment docs.

@mtrezza
Copy link
Member

mtrezza commented Feb 20, 2021

The test fails because the status in step 3 goes from running back to pending.

How does it go from running back to pending? Is there a specific line that causes that?

@dplewis
Copy link
Member Author

dplewis commented Feb 20, 2021 •
edited
Loading

Sorry looking at it again. As @davimacedo said in #7196 there is an error that occurs when updating a status that was just created. So maybe both 2 and 3 are still pending and never got set to running.

@mtrezza
Copy link
Member

mtrezza commented Feb 20, 2021

The conclusion in #7196 (comment) was that the test design caused the flakiness. In this PR it seems that the server code has been changed, that's why I'm wondering.

dplewis
dplewis commented Feb 20, 2021
View reviewed changes
spec/PushController.spec.js
@@ -79,19 +79,6 @@ describe('PushController', () => {
done();
});

it('can throw on validateDeviceType when single invalid device type is set', done => {
Copy link
Member Author

@dplewis dplewis Feb 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm deleting this test as it already exists. Jasmine acts funny with duplicate ids

Screen Shot 2021-02-20 at 2 52 24 PM

@dplewis
Copy link
Member Author

dplewis commented Feb 20, 2021

In this PR it seems that the server code has been changed, that's why I'm wondering.

Thats whats interesting nobody would notice in this in production. If the state went from pending -> succeeded. This PR fixes a potential server bug and a flaky test.

mtrezza
mtrezza approved these changes Feb 20, 2021
View reviewed changes
Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

Coolio!

@mtrezza
Copy link
Member

mtrezza commented Feb 20, 2021

I'm adding this fail to the list "Parse.GeoPoint testing creating geo point exception two fields".

@dplewis dplewis merged commit f1394a9 into master Feb 20, 2021
@dplewis dplewis deleted the flaky-test-jobstatus branch February 20, 2021 21:17
dplewis added a commit that referenced this pull request Feb 21, 2021
@dplewis
Ensure pushStatus is properly running (#7213)
5eec3c4 
* Ensure pushStatus is properly running

* remove duplicate test
dplewis added a commit that referenced this pull request Feb 23, 2021
@dblythy @dplewis
New: allow options to be async on Cloud Validator (#7155)
91a0108 
* new: allow options to be async on Cloud Validator

* Update CHANGELOG.md

* Ensure pushStatus is properly running (#7213)

* Ensure pushStatus is properly running

* remove duplicate test

* new: allow options to be async on Cloud Validator

* Update CHANGELOG.md

* Update CloudCode.Validator.spec.js

Co-authored-by: Diamond Lewis <findlewis@gmail.com>
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.0.0-beta.1

@parseplatformorg parseplatformorg added the state:released-beta Released as beta version label Nov 1, 2021
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.0.0

@parseplatformorg parseplatformorg added the state:released Released as stable version label Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtrezza mtrezza mtrezza approved these changes

@davimacedo davimacedo Awaiting requested review from davimacedo

Assignees
No one assigned
Labels
state:released Released as stable version state:released-beta Released as beta version
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dplewis @mtrezza @parseplatformorg