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

Add suite start/finish events to listener #1118

Closed
wants to merge 1 commit into from
Closed

Conversation

baev
Copy link
Contributor

@baev baev commented Apr 8, 2015

Hi guys!

This pull request adds ability to handle suite start/finish events via RunListener.

Maybe better to use testStarted and testFinished events instead new ones but I am afraid to break backwards compatibility. Tests will be added later.

What are you guys think?

@@ -358,6 +358,7 @@ public Description getDescription() {
public void run(final RunNotifier notifier) {
EachTestNotifier testNotifier = new EachTestNotifier(notifier,
getDescription());
testNotifier.fireTestSuiteStarted();
Copy link
Member

Choose a reason for hiding this comment

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

Note that ParentRunner is the base class of Parameterized. I'm not sure if people would find the events sent from Parameterized confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I see

suite started my.company.MyParameterizedTest
suite started [0]
test started
test
test finished
suite finished
suite started [1]
test started
test
test finished
suite finished
suite finished

Any ideas how to fix this?

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 guess it should looks like

suite started my.company.MyParameterizedTest
test started
test
test finished
test started
test
test finished
suite finished

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if junit creates suite for each parameter maybe it's ok?

Ok, I see

suite started my.company.MyParameterizedTest
suite started [0]
test started
test
test finished
suite finished
suite started [1]
test started
test
test finished
suite finished
suite finished

Copy link
Member

Choose a reason for hiding this comment

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

It might be easier to determine if this is a problem (and if so, the best way to address it) if you could describe the problem you are trying to solve with this change and/or how you plan to use the API

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 am a developer of Allure Framework. We are using RunListener to collect information about tests. This change can help me to get more information about test suites - as example get the right start/stop times for it.

BTW the problem with parameterized runner above -is not a problem for me, but I would like to choose the right way

Copy link
Member

Choose a reason for hiding this comment

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

Well, since we can in theory report errors for the nested suites in Parameterized this is okay with me. I want to wait for the other maintainer to see if they have concerns.

I am curious to find the other maintainers plan on using RunListener for JUnit Lambda, and if so whether this change would work.

@baev
Copy link
Contributor Author

baev commented Apr 8, 2015

ref #444

@kcooney
Copy link
Member

kcooney commented Apr 26, 2015

@junit-team/junit-committers any objections to this pull?

@@ -45,4 +45,12 @@ public void fireTestStarted() {
public void fireTestIgnored() {
notifier.fireTestIgnored(description);
}

public void fireTestSuiteStarted() {
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add Javadoc that includes @since 4.13 to all new methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you please add Javadoc that includes @SInCE 4.13 to all new methods?

@kcooney done

@baev baev force-pushed the master branch 2 times, most recently from ae3e3a6 to 2cc8260 Compare April 26, 2015 10:43
@@ -45,4 +45,22 @@ public void fireTestStarted() {
public void fireTestIgnored() {
notifier.fireTestIgnored(description);
}

/**
* Notify {@link #notifier} that a test suite is about to start.
Copy link
Member

Choose a reason for hiding this comment

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

It seems odd to use @link to link to a private field. How about this;

"Calls {@link RunNotifier#fireTestSuiteStarted(Description)}, passing the {@link Description} that was passed to the {@code EachTestNotifier} constructor. This
should be called when a test suite is about to be started."

@kcooney
Copy link
Member

kcooney commented Apr 26, 2015

LGTM other than very minor issues with the Javadoc (which I can fix when I merge if you prefer). Sorry for the delay.

Just need one of the other maintainers to add their LGTM and I'll merge this.

@baev
Copy link
Contributor Author

baev commented Apr 26, 2015

@kcooney done

@dsaff
Copy link
Member

dsaff commented Apr 27, 2015

This is a good thing in principle, and the implementation looks sound.

I'd like to add some very clear documentation, however. Not every custom runner that reports a parent node and leaf nodes from getDescription() inherits from ParentRunner, so RunListeners must gracefully handle the situation where a suite node shows up in getDescription, but these new methods don't end up getting called.

Likewise, we should probably add to the documentation of RunNotifier that the new methods are strongly encouraged and helpful, but not required; otherwise we risk essentially "breaking" existing Runners.

@kcooney
Copy link
Member

kcooney commented May 2, 2015

@dsaff if the only concerns are the Javadoc, it might be easier to use the hub tool to get this branch into your local repository, update the Javadoc, squash the commits and push

@dsaff
Copy link
Member

dsaff commented May 4, 2015

@kcooney, I'll try to get some time to do that. That said, @baev, if you have time, you may well finish before I do.

@kcooney
Copy link
Member

kcooney commented Jun 6, 2015

@dsaff It's been a month now. I would like to merge this (after applying the minor fixes). How about you improve the Javadoc after this is merged?

@dsaff
Copy link
Member

dsaff commented Jun 8, 2015

Fair enough.

@marcphilipp
Copy link
Member

@kcooney @dsaff Does one of you guys have enough time to work on this?

@kcooney
Copy link
Member

kcooney commented Jul 3, 2015

@marcphilipp I can take care of it this weekend

@kcooney
Copy link
Member

kcooney commented May 29, 2016

I created #1311 to try to resolve David's concerns about the Javadoc.

kcooney added a commit to kcooney/junit that referenced this pull request Jun 1, 2016
testSuiteStarted() and testSuiteFinished() to make it clear that
not all runners call these methods, but runners that call
the started methods should also call the finished methods.

Closes junit-team#1118
@kcooney kcooney closed this in dc43a04 Jun 1, 2016
@kcooney
Copy link
Member

kcooney commented Jun 1, 2016

Merged (finallty!)

@baev would you please update the release notes at https://github.com/junit-team/junit4/wiki/4.13-Release-Notes ?

@baev
Copy link
Contributor Author

baev commented Jun 1, 2016

@marcphilipp
Copy link
Member

Thanks!

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.

4 participants