I think it would prevent a lot of confusion and be helpful for debugging if it didn't only run the block test. This also seems dangerous for the future since the client will report back 100% complete, when it really wasn't.

CommentFileSizeAuthor
#19 504936-clarify-debug.patch856 bytesboombatower
#13 504936-clarify-debug.patch724 bytesboombatower
#6 504936-client-debug.patch897 bytesboombatower
remove_block_only.patch995 bytesdeekayen

Comments

boombatower’s picture

boombatower commented
Status: Needs review » Postponed (maintainer needs more info)

Seems the script is the better route: #511020: Script to self-test pifr2 client. I like being able to run entire system while writing the code by turning on debug switch.

Any other thoughts on how to do this without loosing that ability?

deekayen’s picture

deekayen commented
Status: Postponed (maintainer needs more info) » Active

I don't understand what #511020: Script to self-test pifr2 client has to do with debug mode only running one set of tests. Just running the block tests isn't really "debugging", so much as development. I think the feedback debug mode provides should be standard logging functionality. If debug checkbox did something special, it would be to abbreviate the tests. As it is now, without debug mode turned on, it's harder to figure out why a test failed.

deekayen’s picture

deekayen commented

The situation that raised this issue is that people volunteered servers. When they turned them on, they checked Debug mode on the client configuration in the web interface, as did I. They rant it, and 5 minutes later it returned a successful self test and the client was enabled on the master. It didn't run all the tests and in most of the cases.

When debug mode was turned off, most people's clients didn't work anymore, they got frustrated, and gave up trying. We lost valuable log information as to why and/or it would fail on a different test in the series because the environment wasn't setup correctly with something like GD or clean URLs from mod_rewrite. Block tests alone won't catch a misconfiguration on the client server for testing GD related tests.

boombatower’s picture

boombatower commented
Status: Active » Needs work

The test for this module needs to only run a single test and I do for dev. Lets go ahead and make it a "development flag" and make all the debug output standard.

Sound like a plan?

boombatower’s picture

boombatower commented

pifr_debug() is used almost exclusively in pifr_server. as such I'll just change the only call in the client and then we can move on to: #514162: Log everything!

boombatower’s picture

boombatower commented
Assigned: Unassigned » boombatower
Status: Needs work » Needs review
StatusFileSize
new 504936-client-debug.patch897 bytes

Seems to be the only change required.

boombatower’s picture

boombatower commented
Title: Allow all tests in debug mode » Do not use pifr_debug on client
Status: Needs review » Fixed

Committed.

deekayen’s picture

deekayen commented

Change the checkbox title from Debug to Development, too?

boombatower’s picture

boombatower commented

It still prints debug stuff on server, and client now just always logs which seems best.

deekayen’s picture

deekayen commented
Status: Fixed » Needs work

Kieran just got tricked by the debug checkbox again, too, and he's been working with it for weeks. I don't think #6 alone solves the problem of telling people that checkbox will only run partial tests.

03:15 amazon: deekayen: seems when I run tests manually using http://vps05.hangarservers.net/pifr_client.php only 194 block test cases are run.
03:15 deekayen: amazon: debug mode
03:17 amazon: ok, turned off debug mode
03:19 amazon: Client: testing [659 assertions

Amazon’s picture

Amazon commented

In debug mode, I would expect to see a lot more logging to the watchdog.

What you get is: "pifr_client 07/12/2009 - 07:19 review.runTests() testingadmin"

Debug mode should either log a message that block tests were run successfully, or more obviously, all the tests classes should log messages to the watchdog so admins can see how far the tests ran.

If you want to keep debug mode, rename it to sample test mode or something more accurate.

boombatower’s picture

boombatower commented

I'll just remove the checkbox from the settings page...and add more logging when tests run. I am really not sure how useful that is since you need to look in the assertions table anyway to see where it stopped and why. That shows you the test and reason.

boombatower’s picture

boombatower commented
Status: Needs work » Needs review
StatusFileSize
new 504936-clarify-debug.patch724 bytes
boombatower’s picture

boombatower commented
Status: Needs review » Fixed

Committed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

deekayen’s picture

deekayen commented
Status: Closed (fixed) » Needs work

This is an absolutely evil checkbox. I just caught DamZ's client configuration with the debug checkbox turned on for the client he used to claim that #540386: sqlite not initializing tests was fixed, even though it had only run the block tests. Patch #13 isn't anywhere close to what #12 said.

Please just kill the checkbox dead, move it somewhere secret and out of the way, rename it to "YOU WILL KILL A KITTEN BY CLICKING THIS. SERIOUSLY - THE SPCA WILL BE NOTIFIED."

deekayen’s picture

deekayen commented

Apparently DamZ only wanted block tests. How about renaming it to "Just run the block tests"...

boombatower’s picture

boombatower commented

Yes, I explained to DamZ that he should turn that on so only it runs.

boombatower’s picture

boombatower commented
Status: Needs work » Needs review
StatusFileSize
new 504936-clarify-debug.patch856 bytes

It is mainly for the master server, but he is more clarity. If we really must remove it that is fine, but I like being able to trigger it locally and when you just want to see if a client can even run tests (as in get that far).

boombatower’s picture

boombatower commented
Status: Needs review » Fixed

Committed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.