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 against multiple versions of Postgres #7176

Merged
merged 15 commits into from
Feb 11, 2021
Merged

Test against multiple versions of Postgres #7176

merged 15 commits into from
Feb 11, 2021

Conversation

cbaker6
Copy link
Contributor

@cbaker6 cbaker6 commented Feb 11, 2021

New Pull Request Checklist

Issue Description

Currently, only Postgres 11 is tests against in CI. There multiple supported versions of Postgres and testing against them allows for the community to quickly discover if the Parse server isn't compatible.

Related issue: #7161

Approach

Similar approach to #7161. Since the Parse Server requires Postgis for geo queries, the Postgis images are used. This PR currently tests agains the latest Postgis 3.11 and the previous minor version 3.03.

One difference is since the Postgres CI's use a docker image, we don't need to check to see if the CI is using the latest version since the latest minor version is always pulled from docker. When a new major version is released, this will need to be added to the CI manually.

TODOs before merging

  • Add postgres versions
  • Add latest postgis versions
  • Add entry to changelog
  • Add Postgres versions to docs

@codecov
Copy link

codecov bot commented Feb 11, 2021

Codecov Report

Merging #7176 (1b75fc4) into master (4a3815d) will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7176      +/-   ##
==========================================
+ Coverage   94.00%   94.03%   +0.02%     
==========================================
  Files         172      172              
  Lines       12834    12834              
==========================================
+ Hits        12065    12068       +3     
+ Misses        769      766       -3     
Impacted Files Coverage Δ
src/RestWrite.js 94.00% <0.00%> (+0.32%) ⬆️
src/Adapters/Files/GridFSBucketAdapter.js 80.32% <0.00%> (+0.81%) ⬆️

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 4a3815d...65858a8. Read the comment docs.

@mtrezza
Copy link
Member

mtrezza commented Feb 11, 2021

I must say with 12 tests (+2 more to come for Nodejs versions), these flaky tests become increasingly painful.

@cbaker6
Copy link
Contributor Author

cbaker6 commented Feb 11, 2021

I must say with 12 tests (+2 more to come for Nodejs versions), these flaky tests become increasingly painful.

Definitely, I ran it a couple times on my personal repo before merging and had no issue. As soon as I make the PR here, it fails, lol

@cbaker6
Copy link
Contributor Author

cbaker6 commented Feb 11, 2021

One thing I hate about the matrix in Actions is that when 1 test fails, it automatically cancels all the rest

@cbaker6
Copy link
Contributor Author

cbaker6 commented Feb 11, 2021

@mtrezza It looks like this may be ready for review as it should pass everything when ignoring the flakey tests.

Let me know if you see something I missed

@mtrezza
Copy link
Member

mtrezza commented Feb 11, 2021

I think we should at least get an all pass here before merge.

@cbaker6
Copy link
Contributor Author

cbaker6 commented Feb 11, 2021

I still need to add the badges, will do in a few

@cbaker6
Copy link
Contributor Author

cbaker6 commented Feb 11, 2021

Just in case we get stuck here forever, we had a couple of "all postgres pass", but "mongo failed due to flakey test" happen

@cbaker6
Copy link
Contributor Author

cbaker6 commented Feb 11, 2021

Something I think is interesting is that on my personal repo, it passed 6/7 times with the flakeyness showing up once. I cancelled the 8th run, cbaker6#1

@mtrezza
Copy link
Member

mtrezza commented Feb 11, 2021

Yes, I already stopped looking for patterns in the flaky tests, they are all over the place. I think something with the general test setup.

README.md Outdated Show resolved Hide resolved
@dplewis
Copy link
Member

dplewis commented Feb 11, 2021

I must say with 12 tests (+2 more to come for Nodejs versions), these flaky tests become increasingly painful.

Also Postgres 14 and 15 will probably come out before 10 reaches end of life.

I'm browsing through the limits of Github Action self hosted jobs. We have 20 concurrent jobs. At this rate we will probably have 2 PR going at the same time and the rest will be queued. I think we should understand the limits first before making decisions.

@mtrezza
Copy link
Member

mtrezza commented Feb 11, 2021

I'm browsing through the limits of Github Action self hosted jobs. We have 20 concurrent jobs. At this rate we will probably have 2 PR going at the same time and the rest will be queued. I think we should understand the limits first before making decisions.

Good point. Maybe we can make it phased, testing against less versions on PR commit, but all versions on merge?

I just added another PR #7177 that adds a test against Node 14, in addition to the existing Node 12 test (all other tests run on Node 10).

@cbaker6
Copy link
Contributor Author

cbaker6 commented Feb 11, 2021

Good point. Maybe we can make it phased, testing against less versions on PR commit, but all versions on merge?

Or we can just choose what versions (I would think the 2 newest of mongo and Postgres) to test against and leave the others to developers

@mtrezza
Copy link
Member

mtrezza commented Feb 11, 2021

I'd suggest to test against all versions at least before release, but the further away we get from a PR, the more difficult bug fixing will become.

Not testing risks a situation in which we have been before the PR that tests against multiple MongoDB versions: Parse Server has unknowingly not been fully compatible with MongoDB latest versions since 2019. We are only testing against 4 MongoDB versions of which 1 will reach EOL in April. Well, let's see where the limits are, maybe we are still fine.

That being said, I think we could shift testing more to the developer, as you suggested. Lately I noticed some PRs that were apparently only opened because someone wanted to test their change and use our CI. The PRs did not use the templates, and there was no communication with the author, they basically ignored my messages. We could expect the developer to tests locally and only do the tests before merge.

@cbaker6
Copy link
Contributor Author

cbaker6 commented Feb 11, 2021

Not testing risks a situation in which we have been before the PR that tests against multiple MongoDB versions: Parse Server has unknowingly not been fully compatible with MongoDB latest versions since 2019. We are only testing against 4 MongoDB versions of which 1 will reach EOL in April. Well, let's see where the limits are, maybe we are still fine.

This is why I mentioned to only test against the newest and threw out an arbitrary number, 2 for each DB. In this case developers will be responsible for testing against older versions if they are interested in using them

@dplewis
Copy link
Member

dplewis commented Feb 11, 2021

Parse Server has unknowingly not been fully compatible with MongoDB latest versions since 2019.

I haven't seen an incompatibility issue posted in a while. Are you saying this because the tests need updated or the actual server itself needs to be fixed?

@mtrezza
Copy link
Member

mtrezza commented Feb 11, 2021

Actual server fix, I included the fix in #7161.

It only affected the query.select() feature, which I stumbled upon and that was the reason I made the PR to test against multiple Mongo versions. I would not be in favor of reducing the number of test environments unless this causes a problem. We would go back to status quo ante that allowed this issue to stay undetected for years.

Following other's release support cycles also gives some planability to developers to manage their maintenance resources. Parse Server doesn't offer much to plan at the moment, the release cycle and breaking changes are arbitrary. I think following other support cycles in terms of compatibility is benefiting developers now since #7161 and soon #7177 and this PR.

@cbaker6
Copy link
Contributor Author

cbaker6 commented Feb 11, 2021

I would not be in favor of reducing the number of test environments unless this causes a problem. We would go back to status quo ante that allowed this issue to stay undetected for years.

How does only testing the newest versions of the DB and possibly even node allow the issue to go undetected for a period of time? If I understood the problem you mentioned, it occurred because the version of Mongo in the CI remained constant and Parse was never tested against a newer release. If we always test against the newest 2, the main branch and the latest release is always tested against the newest (this essentially means testing bleeding edge against bleeding edge dependencies, similar to only testing against the latest versions of Apollo and other dependencies in Packages.lock) This means that older versions will phase out of testing over a period of time. Developers who chose to remain on older versions of Parse server, or simply choose to update their respective servers at a slower rate will be able to tell if the prospective parse server version they are considering was compatible with the Mongo, Postgres, and Node versions that came out at the same time as the parse server, because the testing will show up in the CI history

@cbaker6
Copy link
Contributor Author

cbaker6 commented Feb 11, 2021

We have 20 concurrent jobs. At this rate we will probably have 2 PR going at the same time and the rest will be queued. I think we should understand the limits first before making decisions.

@dplewis from the current numbers you showed, what’s the limit on PRs running consecutively now? It seems like maybe 3-4 instead of 2. What is in the pros/cons of this compared to understandability of compatability that @mtrezza is mentioning? How many PRs are really tested at the same time on average?

My “guess” is that many of the consecutive running tests are run by the same PR that are making additional commits/fixes. This can be minimized if it’s possible to change settings than enable: 1) Developers to cancel a workflow on their respective PR 2) Automatically cancel a running workflow on a PR if a new commit is pushed (Travis use to do something like). The essentially means If 2 PRs are being worked on at the same time, they won’t be queued. If more are being worked on, they have a higher probability of being queued.

Tests also seem to run in between 9 - 16 mins, so the longest a PR will be queued to run seems to be 16 mins

@mtrezza
Copy link
Member

mtrezza commented Feb 11, 2021

How does only testing the newest versions of the DB and possibly even node allow the issue to go undetected for a period of time? If I understood the problem you mentioned, it occurred because the version of Mongo in the CI remained constant and Parse was never tested against a newer release.

Because we also need to look back. In a serious production stack, Parse Server is only 1 component of many that requires maintenance. A single component that forces you to jump on never versions because it disregards reasonable support phases of sub-components can be a pain. Frankly, Parse Server already could do better in terms of fixing bugs. To also say we don't test against market-relevant versions anymore is a step into the wrong direction in my opinion. We are talking about versions that are only 2-3 years old.

As I suggested above, less tests on CI PR commit, full tests before merge into master seems the most reasonable and effective approach to me: testing for maximum amount of versions with minimum amount of job frequency.

@cbaker6
Copy link
Contributor Author

cbaker6 commented Feb 11, 2021

Because we also need to look back. In a serious production stack, Parse Server is only 1 component of many that requires maintenance. A single component that forces you to jump on never versions because it disregards reasonable support phases of sub-components can be a pain. Frankly, Parse Server already could do better in terms of fixing bugs. To also say we don't test against market-relevant versions anymore is a step into the wrong direction in my opinion. We are talking about versions that are only 2-3 years old.

I think the developer needs to take some responsibility. “Everything” can’t be done by this repo, nor should we lead developers to expect this. The test cases are there. We can mention in the docs if you want to test the “newest” version of the Parse server against an older version of “X” that isn’t the latest, you should fork the repo and edit the workflow to ensure the tests pass in your respective forks. They can also look through the history I mentioned about the versions tested against the older dependency versions that came out around the same time

@mtrezza
Copy link
Member

mtrezza commented Feb 11, 2021

I suggest we first go into analysis whether there is actually an issue with too many tests, and if so, look for a solution.

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.

LGTM

@mtrezza
Copy link
Member

mtrezza commented Feb 11, 2021

@cbaker6 Would you mind adding the Node.js badges to the readme as well? I forgot that in the Node.js PR. Let me see where I can find the badge links...

Here they are...

MongoDB Node.js

If you can't find any for postgres, you could make them on shields.io.

@cbaker6
Copy link
Contributor Author

cbaker6 commented Feb 11, 2021

Let me know if you see anything else

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.

LGTM, thanks for also updating the ToC!

Now let's hope these 13 tests pass without much re-running 🤞

@mtrezza
Copy link
Member

mtrezza commented Feb 11, 2021

I'm browsing through the limits of Github Action self hosted jobs. We have 20 concurrent jobs. At this rate we will probably have 2 PR going at the same time and the rest will be queued. I think we should understand the limits first before making decisions.

@dplewis That's a good hint. I hope if we run into a limit that won't reflect in billing? I looked into the docs, but couldn't find out which budget limit we have set on GH.

@mtrezza mtrezza merged commit 3f49d51 into parse-community:master Feb 11, 2021
dplewis pushed a commit that referenced this pull request Feb 21, 2021
* Update ci.yml

* Add Postgis 3.0 test

* remove POSTGRES_MAJOR_VERSION from CI and script

* update docs

* nits

* nit

* Add postgres badges

* Add Postgres to TOC

* Shorten mongo and postgres descriptions

* Add badge for node, update mongo/postgres badges

* Add nodejs to TOC

* fix node js TOC

* Nit

* more nits
@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
@cbaker6 cbaker6 deleted the postgres branch November 22, 2021 18:44
@mtrezza mtrezza mentioned this pull request Mar 12, 2022
@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