Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

package test runner in hhvm-dev packages #93

Open
ghost opened this issue Jan 7, 2015 · 24 comments
Open

package test runner in hhvm-dev packages #93

ghost opened this issue Jan 7, 2015 · 24 comments

Comments

@ghost
Copy link

ghost commented Jan 7, 2015

It'd be nice if the hhvm test runner was packaged in hhvm-dev, so extension testsuites could be run after building them.

@ghost
Copy link
Author

ghost commented Jan 8, 2015

just let me know if this is something that you folks want to do. I was gonna include this in my fedora package, but there's no point if it doesn't get any buy in from the ubuntu/debian packagers.

@JoelMarcey
Copy link

I am surprised that the test runner isn't packaged? :/

cc: @jwatzman @fredemmott

@jwatzman
Copy link
Contributor

jwatzman commented Jan 8, 2015

hhvm-dev is for separate extensions right, which wouldn't get any utility out of our testsuite -- though I guess you could break core functionality with them?

Also, would the runner be useful without the tests, which I don't think it makes sense to package? Or do some extensions have test suites that use our test runner but test their extension specifically?

Am I missing something? (Totally possible :-P)

@ghost
Copy link
Author

ghost commented Jan 8, 2015

shouldn't extensions use the same test runner? Or should they all pick their own? It doesn't actually have to be in hhvm-dev, but i do think it should be packaged somehow, because i don't think extensions should have to come up with their own test runner.

@ghost
Copy link
Author

ghost commented Jan 8, 2015

a specific usecase would be the hhvm-pgsql extension. I plan on bringing in the php tests over for pgsql and pdo_pgsql to that repo, but we need a test runner to be able to run them. I'd like to avoid a manual copy of the test runner if possible.

@jwatzman
Copy link
Contributor

jwatzman commented Jan 9, 2015

Hm yeah I see what you're trying to do now, but I'm still not sure that hhvm-dev is the right place to contain the runner. But it also probably doesn't hurt anything. Strong feelings, anyone else?

@Aatch
Copy link
Contributor

Aatch commented Jan 11, 2015

I think making it available is a good idea, I don't have any strong feelings about where it goes though. hhvm-dev seems like a reasonable place though, given it's intended usage.

@ghost
Copy link
Author

ghost commented Jan 11, 2015

@jwatzman : do you happen to have any better ideas? i think a separate package is overkill personally, but maybe that's just me.

@jwatzman
Copy link
Contributor

OK, I'm convinced. Happy to take a PR for this, should be a one-line change to the packaging script, otherwise will do it when I get a chance.

@ghost
Copy link
Author

ghost commented Jan 13, 2015

i might be able to submit a PR to the debian package, it's probably easy to figure out. The question is, what do we name it? run is not a good name.

hhvm-test-runner ?

@ghost
Copy link
Author

ghost commented Jan 13, 2015

i wonder if the shebang line at the top of the test runner should be changed to
#!/usr/bin/env hhvm
instead of
#!/usr/bin/env php

because atm you'd have to call it explicitly with hhvm /path/to/hhvm-test-runner tests/

@fredemmott
Copy link
Contributor

We do now run a subset of tests using php5 to execute the runner, so that should hopefully be unneccessary

@jwatzman
Copy link
Contributor

Yeah either PHP5 or HHVM should work for the runner. hhvm-test-runner works for me, hhvm-run-tests is also fine, something like that.

@ghost
Copy link
Author

ghost commented Jan 13, 2015

ok, two alternatives:

  • a shell script that finds hhvm and calls the runner
  • a cmake target so you can run make test

@jwatzman
Copy link
Contributor

I'm not sure I follow -- I thought you were just going to copy the existing script into the dev package somewhere? Do you need something else? What am I not understanding? :)

@ghost
Copy link
Author

ghost commented Jan 14, 2015

because to excecute the copied script it will require a call like this:

hhvm /path/to/hhvm-test-runner tests

when it should be:

hhvm-testrunner tests

because the test runner has a shebang line of #!/usr/bin/env php (not hhvm) which
means it won't automatically execute hhvm.

EDIT: the alternative being able to make a test target in cmake that gets included in
the dev CMake files, then you can just run

make test

and that target will iron out the differences by trying to find hhvm itself.

@jwatzman
Copy link
Contributor

because the test runner has a shebang line of #!/usr/bin/env php (not hhvm) which means it won't automatically execute hhvm.

This is deliberate, the test runner itself is compatible with PHP5; this has nothing to do with which HHVM it is currently testing (it makes some effort to locate HHVM to test, and will notably just use which hhvm if it detects it's testing an out-of-tree extension). It doesn't matter which runtime the runner itself is using. Or, again, am I missing something?

@ghost
Copy link
Author

ghost commented Jan 15, 2015

yes, i got that based on what fred said. but since it uses php, then it can't find hhvm by itself when you invoke it. It's too late to try to find it later. so you must invoke with with the hhvm executable as:

hhvm hhvm-test-runner

instead of just:

hhvm-test-runner

@jwatzman
Copy link
Contributor

It will run which hhvm to find HHVM https://github.com/facebook/hhvm/blob/d6577daf89598be8bdaa42d7489950b7fd8da265/hphp/test/run#L117-L118 -- why isn't that sufficient?

@ghost
Copy link
Author

ghost commented Jan 16, 2015

because it's too late by then. the point is calling the test-runner itself with hhvm for regular installs. Unless we're operating under the assumption that php must also be installed alongside hhvm everytime.

@jwatzman
Copy link
Contributor

we're operating under the assumption that php must also be installed alongside hhvm everytime

Ah that's what I missed, if you don't have php the runner may not find a runtime to begin executing with. I think saying that we require PHP5 for this is kinda silly :)

Hm, kinda ugly to just use sed or something to switch it over during install, but I don't think we want to change it over to hhvm for internal purposes? @fredemmott is it possible to do it the other way, what makes sense to you here?

@JoelMarcey
Copy link

@jwatzman @jrobeson Is the point here to be able to run the test runner with only hphp/test/run with no php/hhvm prefix AND no arguments to the runner?

https://reviews.facebook.net/D31215 should be landing soon which, as a small part of it, provides an option to specify the hhvm executable as an argument to the runner (the diff has changed somewhat from what you see in the link as most of the comments and changes have been internal), but the new argument is still there.

@ghost
Copy link
Author

ghost commented Jan 16, 2015

@JoelMarcey : the point is to provide the hhvm test runner to third party extensions, so they can run their own tests without copying the test runner into their own repositories. However, the test runner has a shebang line that points to a php executable, which won't exist when installing hhvm-dev(el) packages without php also being installed.

Internal usage for hhvm doesn't have to change.

EDIT: @jwatzman clarified that the debian packages use update-alternatives to set hhvm as a possible php executable, but not all distributions use (or want to use) such a mechanism

@JoelMarcey
Copy link

@jrobeson Yeah, I just realized that this about how to actually begin running the thing which the diff I referenced doesn't deal with at all.

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

No branches or pull requests

4 participants