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

@requires runtime #1356

Closed
marcioAlmada opened this issue Jul 26, 2014 · 28 comments
Closed

@requires runtime #1356

marcioAlmada opened this issue Jul 26, 2014 · 28 comments
Labels
type/enhancement A new idea that should be implemented

Comments

@marcioAlmada
Copy link
Contributor

So I have some tests that should not run against HHVM runtime and others that should run only with HHVM. This can be easily achieved with:

if(defined('HHVM_VERSION'))   $this->markTestSkipped(); // non HHVM
if(! defined('HHVM_VERSION')) $this->markTestSkipped(); // HHVM only

But would be nice to have a way to skip tests from being run in some contexts following the same pattern as @requires extension <ext-name>:

@requires runtime PHP
@requires runtime HHVM
@requires runtime Hippyvm
@whatthejeff
Copy link
Contributor

This makes sense to me. Do you want to provide a PR, @marcioAlmada?

@marcioAlmada
Copy link
Contributor Author

Yes, for sure :D

@whatthejeff
Copy link
Contributor

PS, did you have a chance to look at #1328?

@marcioAlmada
Copy link
Contributor Author

Nope, I haven't noticed the mentions. Better to fix that before we continue with the runtime feature :)

@sebastianbergmann
Copy link
Owner

We already support @requires PHP <version>. Why not expand that support @requires PHP, @requires HHVM, and @requires HHVM <version>?

@sun
Copy link
Contributor

sun commented Jul 26, 2014

@sebastianbergmann's suggestion sounds great to me. I like it.

In the OP, @marcioAlmada mentioned support for alternative runtimes:

@requires runtime Hippyvm

That won't be possible with the simple approach, as we need to hard-code "PHP" and "HHVM" as possible first parameter names.

However, there are only a handful of alternative engines/runtimes, so extensible flexibility wouldn't be warranted either way. People can file PRs if they want support for another engine.

@marcioAlmada
Copy link
Contributor Author

@sun we could add a RuntimeResolution class to solve both runtime-id and runtime-version in the suggested format.

I like the @sebastianbergmann @requires <runtime-id> <version> idea, it seems more intuitive.

@whatthejeff
Copy link
Contributor

Yep, I like @sebastianbergmann's idea too.

@marcioAlmada
Copy link
Contributor Author

Ok, so the feature will expand current @requires <runtime> <optional-version> annotation without BC breaks and in a way it becomes easy to support new runtimes.

Could we add support for runtime exclusions like this @requires ! <runtime> <optional-version>? Ex:

/**
 * @requires ! HHVM
 */

@sun
Copy link
Contributor

sun commented Jul 28, 2014

support for runtime exclusions

Let's create a separate issue for that. It shouldn't be limited to runtimes; I recently had a similar use-case in which I looked for a "run everywhere except this" — i.e., the reverse of @requires, accepting the same possible parameters, but which are negated + OR'ed together (instead of AND'ed). Based on package nomenclature, that would be @conflicts, but just simply @skip would work, too.

@marcioAlmada
Copy link
Contributor Author

It shouldn't be limited to runtimes; I recently had a similar use-case in which I looked for a "run everywhere except this"

@sun I usually try to create small pull requests so we have less friction to add improvements. Your suggestion deviates from that, but I think it would be beneficial.

@whatthejeff, @sun

Some annotations are currently hardcoded into constants and regexps, if we are going to support an exclusion DSL everywhere then we need to refactor the hardcoded way phpunit handle those annotations.

I still prefer the negation operator instead of an antagonist for @requires. The bang seems intuitive enough to me:

/**
 * @requires PHP <version>
 * @requires ! extension xdebug
 */

@whatthejeff
Copy link
Contributor

I think I prefer something like @skip. Maybe @sebastianbergmann has some input?

@sun
Copy link
Contributor

sun commented Jul 28, 2014

Yes. We're wandering on a very thin line between simple meta data vs. a full-blown macro programming language here. — A ! modifier clearly is a language construct in my book.

In any case, negated requirements are out of scope for this issue; let's move that discussion into a separate issue. (It's possible that there are only a few use-cases, so it might not be worth to implement.)

@marcioAlmada
Copy link
Contributor Author

@whatthejeff No problem, I switched to whatever-it-takes-to-have-the-full-feature mode :D

@sun In case there is no way to negate a @requires, how would we skip it? Back to:

if(defined('HHVM_VERSION'))  $this->markTestSkipped(); // non HHVM

To have a @requires and not having a way to do the opposite seems so half baked. Also, I think that despite we have the annotations, it's nice to have a formal PHP API (not sure if we already have it) like we have for expected exceptions.

$this->requiresExtension($extension, $version);
$this->requiresRuntime($extension, $version);
// antagonic
$this->skipRuntime($extension, $version);

Annotations should be only a thin layer that invokes the PHP API behind it IMMO, so the formal api should come first. At this point @skip makes more sense.

@whatthejeff
Copy link
Contributor

I agree on having a formal API as well 👍

@sun
Copy link
Contributor

sun commented Jul 28, 2014

To have a @requires and not having a way to do the opposite seems so half baked.

Yes, as mentioned before, I (for one) do agree that "negated requirements" for skipping tests more easily would be nice. However, these are really two, very discrete, and very independent issues:

  1. Adding support for the alternative HHVM runtime to @requires
  2. Inventing a completely new way for skipping tests via annotations that do NOT match a "requirement".

This issue here is about (1) and shouldn't require many changes. The most minimal approach would be to simply adjust the regular expression in PHPUnit_Util_Test, like this:

diff --git a/src/Util/Test.php b/src/Util/Test.php
--- a/src/Util/Test.php
+++ b/src/Util/Test.php
@@ -65,7 +65,7 @@ class PHPUnit_Util_Test
-    const REGEX_REQUIRES_VERSION   = '/@requires\s+(?P<name>PHP(?:Unit)?)\s+(?P<value>[\d\.-]+(dev|(RC|alpha|beta)[\d\.])?)[ \t]*\r?$/m';
+    const REGEX_REQUIRES_VERSION   = '/@requires\s+(?P<name>(?:PHP(?:Unit)?|HHVM))\s+(?P<value>[\d\.-]+(dev|(RC|alpha|beta)[\d\.])?)[ \t]*\

:-)

@marcioAlmada
Copy link
Contributor Author

@sun I volunteered myself to implement whatever is better, not just apply a hard coded regex tweak and call it a feature.

@whatthejeff what do you think of adding the "formal" requires|skip php api on a separate PR before we decide what to do with the thin annotations layer?

@whatthejeff
Copy link
Contributor

@marcioAlmada Yeah, sounds good. Let's do it :)

@marcioAlmada
Copy link
Contributor Author

Nice, I'll pull request along the week.

@whatthejeff
Copy link
Contributor

Awesome :) Thanks, @marcioAlmada!

@sebastianbergmann
Copy link
Owner

+1 on the formal API. @requires and @skip make sense to me.

@kelunik
Copy link
Contributor

kelunik commented Aug 27, 2014

Would it be possible to add an annotation to skip tests on old runtime versions?

Possible API:

/**
 * @requires PHP < 5.5
 */
public function testSomething() {
    // ...
}

/**
 * @requires PHP 5.5
 */
public function testSomethingElse() {
    // ...
}

@sebastianbergmann sebastianbergmann removed this from the PHPUnit 4.5 milestone Oct 22, 2014
@keradus
Copy link
Contributor

keradus commented Sep 23, 2015

Possibility to define which tests should run only on PHP or only on HHVM would be really great!

@skip also would be nice!

Any news on any of that features?

@GrahamCampbell
Copy link
Contributor

👍 @keradus

@SoboLAN
Copy link

SoboLAN commented Oct 30, 2015

Any progress on this ? :)

@marcioAlmada
Copy link
Contributor Author

wow, I completely forgot this thread :) I'm gonna start working on this ASAP

Somehow I was unsubscribed and missed your pings.

@keradus
Copy link
Contributor

keradus commented Oct 31, 2015

HHVM also provides PHP version, which is supposed to be compatible with equivalent PHP from Zend.

It would be great to allow test to run under:

  • PHP compiler using at least given version, but not HHVM compiler
  • HHVM compiler using at least given version, but not native PHP compiler
  • PHP language using at least given version, under any compiler

@sebastianbergmann
Copy link
Owner

PHPUnit is only supported on PHP. Therefore I see no point in implementing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement A new idea that should be implemented
Projects
None yet
Development

No branches or pull requests

8 participants