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

Refactoring Suite, Parameterized and class runners to allow reuse in custom Runners #1348

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

PeterWippermann
Copy link
Contributor

As discussed in #1338 I refactored some of runners to be able to reuse their functionality. This mainly included the conversion of private instance methods to public (static) class methods. When functionality was hidden in other methods I also introduced new methods (e.g. Parameterized.normalizeParameter(Object)).

@kcooney
Copy link
Member

kcooney commented Jul 20, 2016

Could you please remove all the unnecessary reformatting? It makes it hard to review.

On first glance, the methods being added to BlockJUnit4TestRunner and Suite are so small that the cost of you copying the code is much smaller than the cost of us maintaining the new API.

allParameters() could be a new non-static method in TestClass

I don't understand why you need the changes to ParentRunner. Wouldn't your new runner extend ParentRunner?

@PeterWippermann
Copy link
Contributor Author

@kcooney How should I remove the formatting? Sorry, but Eclipse always formats the whole file and it's not my fault that this file wasn't formatted accordingly before :-/

@kcooney
Copy link
Member

kcooney commented Jul 20, 2016

Tell Eclipse to not format files on save (or, if possible to only reformat changes lines). I realize the files might not follow the current formater settings, but reviewing code that is reformatted in the same pull is painful

@PeterWippermann
Copy link
Contributor Author

Maybe we should have another PR to format all files under org.junit.* once and for good?

@PeterWippermann
Copy link
Contributor Author

PeterWippermann commented Jul 20, 2016

Regarding your feedback, @kcooney :

  • I'd like to reuse functionality which easily finds and processes annotations for me. For that cause BlockJUnit4ClassRunner.getTestRules(Object, TestClass) and Suite.getSuiteClasses(Class<?>) are very handy. I would like to reuse them without copying them. As @SuiteClasses and @TestRule are tightly related to these classes, I think such methods should be available there.
  • Parameterized.allParameters(TestClass) uses other private methods of Parameterized. It's especially depending on @Parameters, which is nested in Parameterized. So shouldn't this "annotation-processing" method also be there?
  • Regarding ParentRunner: Yes, you're right. Those methods could be protected instance methods as well. Would work for me, because my ParameterizedSuite extends ParentRunner indeed. However, do you want to force a user to extend that class to be able to access this functionality?

Would it appeal more to you, if I move all those annotation-related methods to a single (or multiple?) helper class? Its constructor could be initialised with an instance of TestClass. So you instantiate this annotation helper once and have unified access to the annotations then. How should we name it?

@kcooney
Copy link
Member

kcooney commented Jul 20, 2016

I personally prefer that we don't do a pass of reformatting all the files, because it creates conflicts for anyone with an open pull. Perhaps @marcphilipp feels differently. In any case, I'll try to continue the discussion, but it would be much easier without so many extraneous diffs.

Addressing your bullets in order:

  • The amount of reuse is small, and having public static methods to share it (in these classes or other classes) is a bit ugly. Perhaps there is a natural set of classes to extract. In any case, some of these proposed methods have very little code, so the benefit of making it sharable should be weighted against the cost (API we must support, difficulty when we want to make functional changes to the classes, etc)
  • Perhaps we can make a subclass of TestClass named ParameterizedTestClass. Hanging functionality as methods on an object with data is better than a bunch of methods
  • ParentRunner is designed to be subclassed, and developers are encouraged to subclass it for runners that have a parent-child relationship.

*/
public static Statement withBeforeClasses(Statement statement, TestClass testClass) {
protected Statement withBeforeClasses(Statement statement) {
Copy link
Member

@kcooney kcooney Jul 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why you want to call these directly vs. calling classBlock(). If you cannot use classBlock() then perhaps instead of making these three protected we should extract one protected method that calls all three so 1) your runner isn't hard-coding the order and 2) we can add more calls in the future (ex withClassFixtures()). Perhaps call the new method withClassStatements()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like your proposal to introduce withClassStatements(). I also had something like this in mind before, but didn't brought it to an end. I will incorporate it now.

I'd like to propose to leave withBeforeClasses(...) (and the others) protected anyway. But the documentation should encourage to use withClassStatements().

I can't call ParentRunner.classBlock(...) in my custom Runner because between invoking the children and applying the class statements, I'd like to apply @TestRules. ParentRunner doesn't support these.

        Statement statement = childrenInvoker(notifier);

        List<TestRule> testRules = BlockJUnit4ClassRunner.getTestRules(testInstance, getTestClass());
        statement = BlockJUnit4ClassRunner.withTestRules(testRules, getDescription(), statement);

        statement = withClassStatements(statement);
        return statement;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PeterWippermann I don't understand what you mean by "I can't call ParentRunner.classBlock(...) in my custom Runner because between invoking the children and applying the class statements, I'd like to apply @testrules". The method withClassRules() applies all of the rules annotated with @ClassRule. You shouldn't invoke rules annotated with TestRule in ParentRunner because there is no instance of the class.

@PeterWippermann
Copy link
Contributor Author

@kcooney Please have a look on my updates :-)

@kcooney
Copy link
Member

kcooney commented Jul 22, 2016

I'm sorry, but if you want me to spend more time on this you will need to remove all the extraneous reformatting

@PeterWippermann
Copy link
Contributor Author

Which files are you complaining about, @kcooney ?

  • BlockJUnit4ClassRunner is indeed a victim of formatting the whole file
  • the "@@ -1,130 +1,130 @@ diff" of Suite is a bug of Github's display, IMO. Same for BlockJUnit4ClassRunnerWithParameters. I don't see any format differences there.

I really recommend to format your code base once to meet your own code style.

@PeterWippermann
Copy link
Contributor Author

I reported #1350 to get the code style right. Afterwards comparing the code should be easy again.

@kcooney
Copy link
Member

kcooney commented Jul 24, 2016

The problem with Suite is likely due to the line end changes. Perhaps your editor is changing the line endings. You can easily fix it in Emacs; see https://www.emacswiki.org/emacs/EndOfLineTips

We've done passes in the past to reformat the code; it doesn't stick. It also causes merge conflicts for pending pulls.

In any case, all the maintainers work on JUnit in our free time, so we have limited time to review pulls. Having a significant amount of reformatting that is unrelated to your functional changes makes it take longer to review the diffs.

@PeterWippermann
Copy link
Contributor Author

You were right!
I reformatted Suite and BloackJUnit4RunnerWithParameters with Unix line endings and UTF-8 as explained here. The extraneous diffs are gone now.

Still, I'm wondering why not all the files I touched got polluted like this.
The coding style guide should contain an advice for these settings as well.

@PeterWippermann
Copy link
Contributor Author

@kcooney Could you have another look on this please?

@kcooney
Copy link
Member

kcooney commented Aug 16, 2016

@PeterWippermann I am sorry, but my new job has kept me quite busy.

There are still a lot of extraneous diffs in this pull. That makes it take longer for me to re-review it when you ping the thread, and like all of the maintainers, I work on JUnit in my free time (which is very limited currently).

@PeterWippermann
Copy link
Contributor Author

Waiting for #1350

@PeterWippermann
Copy link
Contributor Author

For the period before this PR can be merged, I set up a project that provides exactly this functionality and makes it available for reuse: https://github.com/PeterWippermann/parameterized-suite

@marcphilipp marcphilipp changed the base branch from master to main June 21, 2020 17:05
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 this pull request may close these issues.

2 participants