Tests: Drop openssl CONFLICTS file#22472
Conversation
|
Wouldn't it be better to make the openssl and mysqli test parallel, rather than faffing with tests that might need to be slow by necessity? (it might even be possible to just remove the conflict file for ext/openssl now that ephemeral ports are used) |
|
The second one is actually slow because the tested behavior is to break the program when memory isn't enough. We could just lower the memory amount to break it way earlier though. The first one is similar (it's testing max execution time. We don't need to wait for the default max execution time) However, I do agree we can just remove the conflict file for ext/openssl now. Let's do it. |
c354be8 to
f5fe764
Compare
It is faster for a couple of seconds for every OpenSSL and mysql tests as observed. |
|
Probably makes sense to split the openssl test changes into its own PR. |
f5fe764 to
a001157
Compare
|
I split it. See #22499 |
|
See e1ec67a. This was a conscious decision, as these tests caused more failures with parallelization. We do have the retry mechanism now, I don't know if that's enough. |
|
@iluuu1994 I was aware. My point is to change the conflict key from all to mysql. Since wdyt? This is also the same in #22499 |
|
See https://github.com/php/php-src/blob/master/ext/mysqli/tests/CONFLICTS. This has no effect, the conflict key is already there. The |
|
I see. I thought this is solved because we now use ephemeral ports. Given the uncertainty of the result of this PR. I think we can just return to the original idea to lower the ini setting to avoid unnecessary waiting. I am closing #22499 |
|
Ephemeral ports solve a different issue (different servers being spun up in parallel). There were different efforts to parallelize tests (avoid conflicting table names), but it looks like the CI change was never merged (i.e. removing the CONCLIFCTS file). Maybe @Girgias remembers why that is. |
I don't, but the openssl tests work without the CONFLICT file, something @ndossche told me. |
I've been running the openssl test suite hundreds of times with -j64 in parallel (no exaggeration) with the CONFLCITS file removed, and didn't encounter any issue. |
|
Given the results of Nora's test, is it safe to drop the openssl CONFLICTS file? @iluuu1994 (any objections?) |
|
Keep it in different PR (or at least a different commit when merging). CI is very different to local system in terms of resources so we might need to revert it if we see any failure... |
|
Although maybe leave it for later. I will have some changes comming and still stabilizing the crypto status one so want to do this once all things stabilize and CI is green for some time so I don't need to get disturbed by unrelated failures... |
OpenSSL tests use ephemeral ports now, so the directory-level conflict file is no longer needed.
eeb3575 to
c7c5145
Compare
|
@bukka I directly switch this PR to remove the CONFLICT file (drop the previous one) so if you want to commit feel free to directly push to here. |
No description provided.