Skip to content

Fields annotated with @Rule should be checked to be non-static #339

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

Closed
marcphilipp opened this issue Oct 10, 2011 · 10 comments · Fixed by #343
Closed

Fields annotated with @Rule should be checked to be non-static #339

marcphilipp opened this issue Oct 10, 2011 · 10 comments · Fixed by #343

Comments

@marcphilipp
Copy link
Member

Currently (in 4.10) fields annotated with @ClassRule are checked to be static (like methods annotated with @BeforeClass/@AfterClass).

Fields annotated with @Rule are not checked to be non-static. However, methods annotated with @Before/@After are checked to be non-static.

IMHO this is inconsistent. I don't see any reason why one would allow static fields with @Rule annotation. If you agree, I would be glad to supply a patch via pull request.

@dsaff
Copy link
Member

dsaff commented Oct 11, 2011

Actually, this was somewhat on purpose. For example, imagine that you wanted to take a screenshot of failing tests. You could:

public class Screenshot {
  public static TestRule takeScreenshots = Screenshots.storeFailuresIn("/tmp/some/folder");
}

public class TestA {
  public static @Rule takeScreenshots = Screenshot.takeScreenshots;
}

public class TestB {
  public static @Rule takeScreenshots = Screenshot.takeScreenshots;
}

@dsaff dsaff closed this as completed Oct 11, 2011
@marcphilipp
Copy link
Member Author

Why do the fields have to be static then?

public class Screenshot {
  public static TestRule takeScreenshots = Screenshots.storeFailuresIn("/tmp/some/folder");
}

public class TestA {
  @Rule public TestRule takeScreenshots = Screenshot.takeScreenshots;
}

public class TestB {
  @Rule public TestRule takeScreenshots = Screenshot.takeScreenshots;
}

Wouldn't this work as well?

@dsaff dsaff reopened this Oct 13, 2011
@dsaff
Copy link
Member

dsaff commented Oct 13, 2011

Good point. And the Before/After point is well taken.

OK, reopening--sounds good to me.

marcphilipp referenced this issue in marcphilipp/junit Oct 16, 2011
Fixes KentBeck/junit#339
@stefanbirkner
Copy link
Contributor

Why should we add a restriction, which is not necessary?

@marcphilipp
Copy link
Member Author

Is any of the other restrictions mentioned above necessary?

@dsaff
Copy link
Member

dsaff commented Oct 17, 2011

Well, yes. A @ClassRule has to be static, because it has to exist before any class instances have been created.

To me, the question is whether the number of legitimate uses of a static @rule would be more than the number of mistaken uses of a static @rule. Marc had largely convinced me that static @rules were more dangerous than useful. Stefan, do you disagree?

@stefanbirkner
Copy link
Contributor

Are static @rule really dangerous? I don't want to add a restriction, that we don't need.

@marcphilipp
Copy link
Member Author

I think they might get easily confused with class rules. Are static @before methods dangerous?

@dsaff
Copy link
Member

dsaff commented Oct 17, 2011

Stefan,

If someone says "@rule static...", there's three possibilities:

  1. There's no semantic difference between this and leaving off the static.
  2. There is a semantic difference, and it's one the user intended.
  3. There is a semantic difference, and it's unintended.

I can imagine situations where #3 would happen, especially where the user meant @ClassRule, and misunderstood or mistyped. Can you imagine situations where #2 holds?

@stefanbirkner
Copy link
Contributor

I can imagine rules, which where item 2 holds, but this rules have a bad design and you can easily avoid them. Therefore I give up defending static @rule :-)

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 a pull request may close this issue.

3 participants