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

Fix flaky postgres test #7228

Merged
merged 4 commits into from
Feb 25, 2021
Merged

Fix flaky postgres test #7228

merged 4 commits into from
Feb 25, 2021

Conversation

cbaker6
Copy link
Contributor

@cbaker6 cbaker6 commented Feb 25, 2021

New Pull Request Checklist

Issue Description

I introduced a flaky test in #6506 where MD5 was used to create random strings. The problem here is these strings are used for values on a unique index and MD5 generates clashes easily as the strings are not promised to be unique.

Related issue: Flaky tests

Approach

Use postgres gen_random_uuid() instead of MD5 for random strings since they are guaranteed to be unique, especially for the amount generated in the tests (5000).

TODOs before merging

  • Add entry to changelog

@cbaker6 cbaker6 mentioned this pull request Feb 25, 2021
4 tasks
@codecov
Copy link

codecov bot commented Feb 25, 2021

Codecov Report

Merging #7228 (db0d7d9) into master (c4aadc9) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7228      +/-   ##
==========================================
- Coverage   94.03%   94.01%   -0.03%     
==========================================
  Files         172      172              
  Lines       12913    12913              
==========================================
- Hits        12143    12140       -3     
- Misses        770      773       +3     
Impacted Files Coverage Δ
src/batch.js 91.37% <0.00%> (-1.73%) ⬇️
src/ParseServerRESTController.js 97.01% <0.00%> (-1.50%) ⬇️
src/RestWrite.js 93.84% <0.00%> (-0.17%) ⬇️

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 c4aadc9...db0d7d9. Read the comment docs.

@dplewis
Copy link
Member

dplewis commented Feb 25, 2021

Do you have to use an extension for this?

@cbaker6
Copy link
Contributor Author

cbaker6 commented Feb 25, 2021

Do you have to use an extension for this?

Trying to see if I can install the extension directly for Postgres 10 and 11, seems to already be included in 13 (and I think 12)

@dplewis
Copy link
Member

dplewis commented Feb 25, 2021

I just realized that the Contributing Guide need to be updated for testing against Postgres like creating database and proper extensions.

@cbaker6
Copy link
Contributor Author

cbaker6 commented Feb 25, 2021

The contributing guide shows how to add the correct extensions for regular usecases

@dplewis
Copy link
Member

dplewis commented Feb 25, 2021

Which line?

@cbaker6
Copy link
Contributor Author

cbaker6 commented Feb 25, 2021

Which line?

137 to 139

I always copy paste from the Contributing guide when testing to make sure it's still valid

@dplewis
Copy link
Member

dplewis commented Feb 25, 2021

I see, I was looking at the old travis reference that is in there.

@cbaker6
Copy link
Contributor Author

cbaker6 commented Feb 25, 2021

I see, I was looking at the old travis reference that is in there.

I can update this here since I'm adding the pgcrypto extension

@cbaker6
Copy link
Contributor Author

cbaker6 commented Feb 25, 2021

@dplewis test look all good. I'm assuming I don't need to add a changelog entry for a test fix?

CONTRIBUTING.md Outdated Show resolved Hide resolved
scripts/before_script_postgres.sh Show resolved Hide resolved
Copy link
Member

@dplewis dplewis left a comment

Choose a reason for hiding this comment

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

Nice!

@dplewis dplewis merged commit 2b9b336 into parse-community:master Feb 25, 2021
@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 postgresTest branch November 22, 2021 18:43
@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.

3 participants