For people who have run command line scripts before, run-tests.sh is very disorienting.

It's executable, so you'd think you could execute it, but you can't because there's no shebang. We give it a .sh extension, but it cannot be invoked with sh. Requiring someone to run php core/scripts/run-tests.sh is very unintuitive.

The attached patch removes the extension and adds a #!/usr/bin/env php shebang which is much more common. Now you'd just run:

./core/scripts/run-tests

From what I've read, the .sh extension was used instead of .php so that web servers wouldnt execute it. With no extension, it won't be executed as php, it'll just be served as text. It would make a site easier to fingerprint, but we don't deny access to README.txt, so I'm not seeing that as a problem.

CommentFileSizeAuthor
#3 runtests-use-shebang-do-not-test.patch305 bytesmsonnabaum
runtests-use-shebang.patch41.71 KBmsonnabaum

Comments

Status: Needs review » Needs work

The last submitted patch, runtests-use-shebang.patch, failed testing.

damien tournoud’s picture

damien tournoud commented

Please roll this patch with git [diff or show] -C -M.

We voluntarily didn't add the shebang initially, because we used to fork the main process so all the tests would end-up being executed with whatever PHP executable is the default on the system. This caused a lot of trouble with systems in which the PHP executable used to run Drupal via the web interface is different then the system PHP (meaning: Windows and most of the MacOS X installations; ie. a really large portion of the development environments).

Now that we launch a subprocess instead of forking it probably doesn't really matter anymore, as long as the system PHP is able to bootstrap Drupal.

msonnabaum’s picture

msonnabaum commented
Status: Needs work » Needs review
StatusFileSize
new runtests-use-shebang-do-not-test.patch305 bytes

Here's the patch with a proper rename and it's set to not test since I'm assuming there's no way for the bot to not fail on this.

damien tournoud’s picture

damien tournoud commented

Ok, so either we go into a painful deployment process of a new version of the test bot or don't do the rename and just add the shebang :)

msonnabaum’s picture

msonnabaum commented

What about keeping run-tests.sh and having it just include run-tests? Would make backwards compatibility less painful.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Yes, for the time being I'd prefer a separate file:

run-tests

#!/usr/bin/env php
<?php
// @todo Actually, how do we get to the relative path here?
require_once ./run-tests.sh

So in light of that @todo, perhaps rather this?

run-tests

#!/usr/bin/env php

php run-tests.sh $*

EDIT: Scratch that. Of course, it is:

run-tests

#!/usr/bin/env php
<?php
require_once dirname(__FILE__) . '/run-tests.sh';
tr’s picture

tr commented

This has been an open issue for a long time: #655178: [PP-2] Rename run-tests.sh to run-tests.php

I agree with the goal and don't want to shut down the discussion, but one of these issues needs to be marked as a duplicate and closed. I'll leave it to the OP to decide which one.

alansaviolobo’s picture

alansaviolobo commented
Issue summary: View changes
Status: Needs review » Needs work
joachim’s picture

joachim commented
Status: Needs work » Closed (duplicate)

I'm going to say this one should be closed, as the other was updated more recently, and has discussions about integrating with Drush too. The patches on both issues are pretty similar.