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

Suite runner extensibility #487

Merged
merged 15 commits into from Aug 17, 2014
Merged

Suite runner extensibility #487

merged 15 commits into from Aug 17, 2014

Conversation

@fhoeben
Copy link
Collaborator

@fhoeben fhoeben commented Jun 28, 2014

These changes would allow much easier customization, to resolve #486

@fhoeben
Copy link
Collaborator Author

@fhoeben fhoeben commented Aug 3, 2014

Can I get some feedback on this request? Any collaborator willing to merge this, or should I make additional changes, or do you believe it's a bad idea?

@amolenaar
Copy link
Collaborator

@amolenaar amolenaar commented Aug 4, 2014

Hi,

What's the case for the abstract FitNesseRunner? Can this be merged back with FitNesseSuite (maybe use composition instead of inheritance to separate concerns).

Loosening the methods from package private to protected seems fine. I do like the test case.

@fhoeben
Copy link
Collaborator Author

@fhoeben fhoeben commented Aug 5, 2014

My idea for the abstract base class was indeed to make a single class focus on actually running the tests and another on how to configure what to run.
I did consider composition but moving the annotations to a separate (new) configurator class would break existing code. I will have another close(r) look.

… 'Runner'.

Furthermore introduce @suite annotation as replacement for @name
@fhoeben
Copy link
Collaborator Author

@fhoeben fhoeben commented Aug 6, 2014

I've changed the FitNesseRunner to no longer be abstract, but to be a full replacement of FitNesseSuite, which I made deprecated. I also added tests to see that FitNesseSuite usage still works as before.

How do you feel about this?

@amolenaar
Copy link
Collaborator

@amolenaar amolenaar commented Aug 6, 2014

Now we have a more sensible name for the junit runner. That makes sense.
The methods are now better accessible so you can create a specific subclass if you have to.
I think that's nice.

@fhoeben
Copy link
Collaborator Author

@fhoeben fhoeben commented Aug 6, 2014

Does that mean you will merge it in?

@amolenaar
Copy link
Collaborator

@amolenaar amolenaar commented Aug 14, 2014

Yup :)

Can you check the build (perform an "ant release"?). I get a compilation error when trying to compile your branch.

@amolenaar amolenaar added this to the Next release milestone Aug 14, 2014
@fhoeben
Copy link
Collaborator Author

@fhoeben fhoeben commented Aug 14, 2014

I found out I completely missed the 'test' directory for junit tests, sorry.
I moved my tests to this location and addressed the compilation problems.For me locally not all tests pass, but quite a few seem quite distant from my changes so I hope these are caused by an error in my setup...

@amolenaar amolenaar merged commit 10f9a14 into unclebob:master Aug 17, 2014
amolenaar added a commit that referenced this pull request Aug 17, 2014
@fhoeben fhoeben deleted the fhoeben:SuiteRunnerExtensibility branch Aug 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants