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

Make it easier for scripts to retrieve passed and skipped test results #24

Merged
merged 12 commits into from Dec 10, 2015

Conversation

@aroskuski
Copy link
Contributor

aroskuski commented Aug 21, 2015

Currently, it is very simple to get a list of failed tests with a single line of code by calling getFailedTests() on the TestResultAction. However, getting a list of passed or skipped tests is significantly more difficult. To do that, you have to dig down to the PackageResult level, which not involves traversing further down into the data structures, but also means that you have to collect the results from each package individually.

The changes in this pull request aim to allow passed and skipped test results to be retrieved just as easily as failed ones by changing the way passed and skipped results are handled internally to be more similar to how failed results are handled.

@jenkinsadmin

This comment has been minimized.

Copy link
Member

jenkinsadmin commented Aug 21, 2015

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

* A shortcut for scripting
*
* @return List of passed tests from associated test result.
*/

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 21, 2015

Member

Add @since TODO and NonNull annotations

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Aug 21, 2015

From my POV the change is reasonable, because it follows the "failedTests" pattern.

The only serious concern is related to the impact on the performance and on the memory size. If we have a big test suite (e.g. 10k tests) in the continuous integration flow, we usually expect several tests to fail due to the recent change, so the number of saved test results is minimal. BTW, saving of passed tests in such frequent builds may bloat the RAM memory.

I would propose to make the behavior alterable somehow. E.g. you could keep the original counters for getXXXCount() and record arrays only if such mode is enabled in TestResult and PackageResult constructors.

@aroskuski

This comment has been minimized.

Copy link
Contributor Author

aroskuski commented Aug 21, 2015

How about I make it so that the lists of passed and and skipped test results are only generated on demand when getPassedTests() or getSkippedTests() are called? That way, they only get generated if the user intends to use them.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Aug 21, 2015

Yes, such approach should work.

@aroskuski

This comment has been minimized.

Copy link
Contributor Author

aroskuski commented Aug 22, 2015

Okay, all your feedback should be addressed.

I'm not 100% sure that I did @Nonnull correctly, however. I've only seen it used on function arguments before.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Aug 30, 2015

Nonnull is OK.

I think that now we can get into concurrency issues, so it makes sense to make new getter methods synchronized.

@aroskuski

This comment has been minimized.

Copy link
Contributor Author

aroskuski commented Aug 30, 2015

How's this?

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Nov 24, 2015

@aroskuski Thanks for fixing the comments. Hopefully I'll test the change and integrate it by the next weekend

@@ -201,7 +201,7 @@ public boolean hasChildren() {
* @return the children of this test result, if any, or an empty collection
*/
@Override
public Collection<? extends hudson.tasks.test.TestResult> getPassedTests() {
public List<CaseResult> getPassedTests() {
List<CaseResult> r = new ArrayList<CaseResult>();

This comment has been minimized.

Copy link
@olivergondza

olivergondza Nov 25, 2015

Member

Is this change not going to break ABI compatibility for existing callers?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Nov 25, 2015

Member

It's an override, so it should be safe

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Nov 25, 2015

👍

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Dec 3, 2015

@olivergondza Ready to go IMHO

oleg-nenashev added a commit that referenced this pull request Dec 10, 2015
Make it easier for scripts to retrieve passed and skipped test results
@oleg-nenashev oleg-nenashev merged commit e4eb245 into jenkinsci:master Dec 10, 2015
1 check passed
1 check passed
Jenkins This pull request looks good
Details
@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Dec 10, 2015

ehh, I've forgotten to squash it :( my bad

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.