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

test: fix failing tests after removal of session token #7599

Merged
merged 3 commits into from
Sep 30, 2021

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Sep 30, 2021

New Pull Request Checklist

Issue Description

Merging security fix 834ae36 introduced failing tests.

Related issue: #n/a

Approach

Fixes failing tests.

TODOs before merging

n/a

@parse-github-assistant
Copy link

parse-github-assistant bot commented Sep 30, 2021

Thanks for opening this pull request!

  • 🎉 We are excited about your hands-on contribution!

@dblythy dblythy changed the title Fix failing tests fix: fix failing tests Sep 30, 2021
@dblythy dblythy marked this pull request as draft September 30, 2021 04:29
@codecov
Copy link

codecov bot commented Sep 30, 2021

Codecov Report

Merging #7599 (7878db9) into master (b86e3bf) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 7878db9 differs from pull request most recent head 8e1eac1. Consider uploading reports for the commit 8e1eac1 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7599      +/-   ##
==========================================
+ Coverage   93.94%   93.96%   +0.01%     
==========================================
  Files         181      181              
  Lines       13289    13289              
==========================================
+ Hits        12485    12487       +2     
+ Misses        804      802       -2     
Impacted Files Coverage Δ
src/Adapters/Files/GridFSBucketAdapter.js 79.50% <0.00%> (-0.82%) ⬇️
src/RestWrite.js 94.10% <0.00%> (+0.31%) ⬆️
src/ParseServerRESTController.js 98.50% <0.00%> (+1.49%) ⬆️

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 b86e3bf...8e1eac1. Read the comment docs.

@dblythy dblythy marked this pull request as ready for review September 30, 2021 05:40
@dblythy dblythy requested a review from mtrezza September 30, 2021 05:40
@dblythy dblythy closed this Sep 30, 2021
@dblythy dblythy reopened this Sep 30, 2021
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.

Looks good, I sense some Swift syntax here 🙂 I take it that the 4.10.4 release is fine, it was just the tests themselves?

I assume these were last-minute changes after the last local test run, that's why they were overlooked? We should probably establish a protocol for the reviewer that says the first step before merging an advisory is running at least one mongodb and one postgres test locally.

@mtrezza mtrezza changed the title fix: fix failing tests test: fix failing tests after removal of session token Sep 30, 2021
@dblythy
Copy link
Member Author

dblythy commented Sep 30, 2021

I take it that the 4.10.4 release is fine, it was just the tests themselves?

Yep! I guess the ? operator isn't available until Node 14.

I assume these were last-minute changes after the last local test run

It was my mistake for running the tests only on Node 16!

We should probably establish a protocol for the reviewer that says the first step before merging an advisory is running at least one mongodb and one postgres test locally.

This sounds like a good idea to avoid this happening in the future. Perhaps we should also have the tests run locally on the same Node versions as the CI

@mtrezza
Copy link
Member

mtrezza commented Sep 30, 2021

Right, so it should probably be at least one local test for:

  • MongoDB
  • Postgres
  • lowest Node version in CI (Node 12)
  • highest Node version in CI (Node 15)

And if backported, such a test for each branch.
At least until GitHub runs CI in advisories.

Is Parse Server already Node 16 compatible, because you said you ran it on Node 16? I thought there was still an open issue?

@mtrezza mtrezza merged commit d90c159 into parse-community:master Sep 30, 2021
mtrezza pushed a commit to mtrezza/parse-server that referenced this pull request Sep 30, 2021
@dblythy
Copy link
Member Author

dblythy commented Sep 30, 2021

Actually I’m mistaken - it would’ve been Node 15 / npm 6 otherwise lockfile would be v2

@mtrezza
Copy link
Member

mtrezza commented Sep 30, 2021

Got it, thanks

@FransGH
Copy link
Contributor

FransGH commented Oct 27, 2021

Could it be that this is not in the 4.10.4 release? Running the tests seem to fail on exactly this issue, see #7658

@mtrezza
Copy link
Member

mtrezza commented Oct 27, 2021

@FransGH yes, this was not merged into the 4.x branch but into master, now alpha. The release-4.x branch is currently maintained as a long-term support branch and only receives bug fixes, like the one you are following. This is the same PR for the 4.x branch, but it has become stale: #7600

@FransGH
Copy link
Contributor

FransGH commented Oct 27, 2021

I'm currently running the tests with node 14, so far seen the same error as in #7600 but they are still running...

Basically looks like the 4.x.x branch won't work with the current test-workflows as these require node 10.
How would a possible future 4.10.x release update work, for example with some critical security fix?

@mtrezza
Copy link
Member

mtrezza commented Oct 27, 2021

Let's discuss in #7658

FransGH pushed a commit to FransGH/parse-server that referenced this pull request Oct 27, 2021
@dblythy dblythy deleted the fix-tests branch October 31, 2021 08:54
@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
Labels
state:released Released as stable version state:released-beta Released as beta version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants