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

Use plaintext for connectivity test #9528

Conversation

WyriHaximus
Copy link

By using plaintext for connectivity testing instead of a DB related one we rule out any issues related to the database while checking if the server is up

By using plaintext for connectivity testing instead of a DB related one we rule out any issues related to the database while checking if the server is up
@joanhey
Copy link
Contributor

joanhey commented Jan 13, 2025

Please don't touch the toolset.
You can test any url individually.

./tfb --help

./tfb --mode verify --test reactphp --type plaintext

@@ -74,7 +74,7 @@ def is_accepting_requests(self):

url = "http://%s:%s%s" % (self.benchmarker.config.server_host,
self.port,
self.runTests[test_type].get_url())
self.runTests["plaintext"].get_url())
Copy link
Contributor

Choose a reason for hiding this comment

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

This change breaks all tests.

Copy link
Author

Choose a reason for hiding this comment

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

Ok then how can I change which test to use for this check for the ReactPHP bench mark?

@WyriHaximus
Copy link
Author

Please don't touch the toolset. You can test any url individually.

./tfb --help

./tfb --mode verify --test reactphp --type plaintext

Sure, but that isn't listed on this repos readme, and when running the tests with ./tfb --mode verify --test reactphp mentioned there it just fails. Looking for a way to see this instead:
image

@joanhey
Copy link
Contributor

joanhey commented Jan 14, 2025

For run all the tests, start with the db test, and reactphp start to be blocked.

Never send a response, not even a 500 error, only show the errors (if configured) in the shell.
Without responses never will continue the tests, as is waiting for them, and even still will fail the rest of tests.

@joanhey
Copy link
Contributor

joanhey commented Jan 14, 2025

image

I need to stop with ctrl+c, because is blocked there and the tests reject it after some time.
Before was worst, as don't show any error in the shell, but it was blocked.

PD: that's before in the run
image
https://tfb-status.techempower.com/unzip/results.2025-01-10-22-35-37-339.zip/results/20250104202231/reactphp/run/reactphp.log

@joanhey
Copy link
Contributor

joanhey commented Jan 14, 2025

Also if the error is for not connect to the database, it'll be good that show that: "Error connecting to the database". And return a 500 error.

@joanhey
Copy link
Contributor

joanhey commented Jan 15, 2025

Mysql versions:
image

So Reactphp started to fail after the update to Mysql 8.4, even with enabled mysql-native-password.
But it'll be good if fail in a more informed way.

PD: check #9009 #9175 #9183

@WyriHaximus
Copy link
Author

For run all the tests, start with the db test, and reactphp start to be blocked.

Never send a response, not even a 500 error, only show the errors (if configured) in the shell. Without responses never will continue the tests, as is waiting for them, and even still will fail the rest of tests.

Fair, I'll have a look at fixing that. But I still think, connection tests should use the simplest end point possible to avoid complexity.

@WyriHaximus
Copy link
Author

Mysql versions: image

So Reactphp started to fail after the update to Mysql 8.4, even with enabled mysql-native-password. But it'll be good if fail in a more informed way.

PD: check #9009 #9175 #9183

Postgres is also support for the benchmarks from what I've seen. Will have a look at switching it to that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants