-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Enable test suite to be randomized #7265
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7265 +/- ##
==========================================
- Coverage 94.01% 93.98% -0.03%
==========================================
Files 179 179
Lines 13140 13144 +4
==========================================
Hits 12354 12354
- Misses 786 790 +4
Continue to review full report at Codecov.
|
@parse-community/core-maintainers I need some help with an issue. I realized that sometimes config.fileController or config.database (databaseController) is undefined. Is there any situation when you start a server the config would be empty or any other way it could be set to empty? I can't remember and will look around, just wanted to ask. I believe this is the last piece to the puzzle. Edit: https://github.com/parse-community/parse-server/blob/master/src/Config.js#L31 |
I remember someone mentioning this in an issue some weeks ago, I think @davimacedo made a comment on it, not sure. In the context of jasmine, I can think of
Maybe the |
@mtrezza I figured out the issue, it was caused by UserController.spec.js. I updated my finding in the OP. |
@davimacedo @mtrezza This is good for review. Let me know if you have any questions. I don’t know how many flaky tests still exist. My final conclusion is 2800+ test and not one server change, that says something. |
Can you explain what that means? |
After spending dozens of hours cleaning up hundreds of poorly written test. I didn’t find a single server bug. At least I have enough knowledge about the test suite to fix the flaky tests next |
Pretty cool! Do you have any general recommendations as to how the test suites are set up or is that principally fine as it is? |
I think it's fine the way it is. Its actually pretty simple when you compare it to the JS SDK.
@mtrezza I found one lol in the LdapAuth.spec.js |
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.
Great job, @dplewis ! It looks all good to me.
* initial run * Update ParseGraphQLServer.spec.js * temporarily enable reporter * Bump retry limit * fix undefined database * try to catch error * Handle LiveQueryServers * Update Config.js * fast-fail false * Remove usage of AppCache * oops * Update contributing guide * enable debugger, try network retry attempt 1 * Fix ldap unbinding * move non specs to support * add missing mock adapter * fix Parse.Push * RestController should match batch.spec.js * Remove request attempt limit * handle index.spec.js * Update CHANGELOG.md * Handle error: tuple concurrently updated * test transactions * Clear RedisCache after every test * LoggerController.spec.js * Update schemas.spec.js * finally fix transactions * fix geopoint deadlock * transaction with clean database * batch.spec.js
🎉 This change has been released in version 5.0.0-beta.1 |
🎉 This change has been released in version 5.0.0 |
New Pull Request Checklist
Issue Description
Per a discussion #7262 (comment)
This is an attempt to run the tests at random. By random means each test file will be ran at random. While a test file is running the test within the file will be randomized. A test may rely on a previously run test and would be deemed poorly written.
Related issue: FILL_THIS_OUT
Approach
Majority of the tests have already been fixed in #7262. This is a first pass attempt. This could be a community effort. If it seems too daunting of a task we can still merge but remove random.
Known Failing Test Suites
No error here just wanted to set the server to different ports. Easier for debugging
The mock adapter was changed and never reset
Config
not properly setProperly close servers and reset
Bug I rewrote the test getting rid of Promise.finally. I then noticed the servers couldn't close on group search error. If there was an error I unbind and destroyed the client similar to how other errors are handled.
Assumed a log would exist.
Bug Ensure multiple geopoint field validation runs before any operation.
Initialized new server beforeEach test and close after. Set unique field names in tests.
Remove the use of AppCache. Changed server port to not conflict with
LdapAuth.spec.js
Properly close servers and reset
Changed server port to not conflict with
index.spec.js
Ensure pushStatus object exists.
Resets the mockEmailAdapter. Remove the use of AppCache. This cause the Config to be improperly set. Normally it would run at the end of the test run alphabetically. In a random test it ran first. Thats how I was able to find this issue.
See: https://github.com/parse-community/parse-server/runs/2103910826 The tests immediately started failing. I don't know why when the cache gets cleared on every reconfigureServer.
Clear cache before every test
The issue was the protectedFields gets merged with defaults. Once merged every subsequent
reconfigureServer
call would use the previous protectedFields from defaults. Caused by https://github.com/parse-community/parse-server/blob/random-tests/src/ParseServer.js#L419These tests timeout I believe this could be a server issue
These tests timeout I believe this could be a server issue
Critical
The most likely cause of this that the JS SDK is a singleton and will get reinitialized when you create a new Server instance. It also gets initialized with the serverURL you set. In our case the servers closed but the serverURL was the same. Meaning all test will use the old serverURL until a new Server instance is created.
Caused when a test timeout the file descriptor is open keeping the server from closing. Similar to the client being alive prevents closing the server in
LdapAuth.spec.js
. I addeddestroyAliveConnections
to mitigate this risk.It was caused by
error: tuple concurrently updated
Nice to have
/support
TODOs before merging