-
Notifications
You must be signed in to change notification settings - Fork 150
fix: macos test failures due to concurrent file access #1312
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
Conversation
- convert nedb file-based database setup to use in-memory databases when running in test mode - remove the single process requirement for tests, run tests in parallel
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1312 +/- ##
==========================================
- Coverage 80.99% 80.85% -0.14%
==========================================
Files 66 66
Lines 4499 4514 +15
Branches 776 778 +2
==========================================
+ Hits 3644 3650 +6
- Misses 840 849 +9
Partials 15 15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jescalada
left a comment
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.
Code changes LGTM! Wondering if this fixes things on your end @kriswest? I've been unable to reproduce the errors you see on the OIDC test...
|
This branch has given me the first clean runs of the main tests, thanks @coopernetes. However, I'm having trouble with the CLI tests and can't get almost any to pass. I'm quite disturbed by this: I can't find the string 'widyun' in the codebase and see no reason that it should be connecting out to a websocket. The connection message happens to cause a bunch of test failures, e.g.: @coopernetes @jescalada do you know anything about this and can you replicate? We may have a dependency that has been compromised! |
|
I can't find the string widyun in the codebase or node_modules anywhere, so it may be obfuscated... |
|
...and after switching and reinstalling many times trying to track down when this starts, it has stopped. Its possible that there was a compromised package version I was picking up but its since been resolved and I'm not picking it up anymore?!? Can others try running the CLI tests and let me know whether you replicate? |
|
@kriswest I recall seeing those errors at some point before... During my CLI TS refactor I was having a workspace resolution issue - it was resolving to an old version of the CLI, not sure which. I've looked through old releases right after SSH was released ( |
|
I'm going to go ahead and merge this since it solved the underlying concurrent file access. For that strange websocket connection, let's track that in a separate issue. Could be some side effects of a corrupted node_modules cache, a legit security issue or some other bug in those CLI packages. I haven't had a chance to try to repro that one Kris reported yet. |
Fixes #1310
in test mode
remove the single process requirement for tests, run tests in parallel