Increase InstrumentationResultPrinter.MAX_TRACE_SIZE #711

pyricau · 2020-08-07T23:02:21Z

This change increases InstrumentationResultPrinter.MAX_TRACE_SIZE from 65,536 bytes to 131,072 bytes, giving more room for long stacktraces (think RxJava and also view hierarchy logged as part of the trace message).

Also attempts to fallback to getTrimmedTrace() before actively trimming the trace at a random point.

googlebot · 2020-08-07T23:02:26Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

pyricau · 2020-08-07T23:03:50Z

Damn, I did sign the corporate CLA with my work email but Github online edit picked my personal email.

bohsen · 2020-08-25T10:35:31Z

@brettchabot Friendly bump. This would be a welcome change also for projects using MaterialComponents. As their View-hierarchies are very large.

brettchabot · 2020-08-25T16:11:02Z

...d_junit_runner/java/androidx/test/internal/runner/listener/InstrumentationResultPrinter.java

@@ -45,7 +45,7 @@

  private static final String TAG = "InstrumentationResultPrinter";

-  @VisibleForTesting static final int MAX_TRACE_SIZE = 32 * 1024;


I'm concerned this change is risky. It looks like the reason this limit exists is to prevent going over the binder IPC limit. It would seem like increasing the trace size would increase the chances of that occurring.

I thought the binder maximum size was 1 MB.

Right but I think it is possible that multiple stack traces will get sent in one binder transaction.

Any value here is risky, in any direction we go. Decreasing max means shorter stacktraces => Espresso is less useful. Increasing the max means more risk of hitting the binder max.

What if this didn't already have a value today, how would we pick a value? 65,536 bytes is super conservative, but 131,072 bytes is also very conservative. To send multiple stacktraces at the same time you'd need to run tests in parallel and you'd need them to fail and you'd need long stacktraces for each. Isn't that a bit far fetched?

I am under the impression that multiple stack traces can get sent in one transaction even under normal conditions. Even with this trace limit in place, there are user complaints about binder size transaction errors.
#269

Long term we hope to move away from using binder to communicate test results.

Would it be hard to impose a limit on the aggregated size rather than individuals? If the cumulative stack trace size is big in one binder transaction, then stop adding more stack traces. This way, the common usefulness will increase (developer running a single test, tying to reproduce an issue and not seeing stack currently), while other cases will have more data too, but with some stacks fully missing.

The proposed change is targeting InstrumentationResultPrinter, not OrchestrationResultPrinter/OrchestrationXmlTestRunListener which the linked issues IMO clearly shows has issues.
Also OrchestrationResultPrinter/OrchestrationXmlTestRunListener do not impose any limits on the MAX_TRACE_SIZE although the OrchestrationResultPrinter-javadoc states:

...
A line by line reimplementation of {@link
androidx.test.internal.runner.listener.InstrumentationResultPrinter}
...

(maybe they actually should)

And they have no dependency on InstrumentationResultPrinter from what I can tell.

Ah cool. Thanks for all the investigation everyone. There have been reports of non-orchestrator-using users hitting binder transaction issues as well, but I suspect they were making heavy use of metrics. This change sounds reasonable but I'd like to do a bit more investigation with those users. I'm pretty slammed this week but will report back next week.

brettchabot · 2020-08-27T16:55:42Z

@pyricau would you be able to sign the CLA? thanks

googlebot · 2020-08-30T16:55:27Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

pyricau · 2020-08-30T16:55:59Z

Amended the commit to replace author with my work email, CLA ✅

brettchabot · 2020-08-31T21:50:15Z

...d_junit_runner/java/androidx/test/internal/runner/listener/InstrumentationResultPrinter.java

-          TAG,
-          String.format("Stack trace too long, trimmed to first %s characters.", MAX_TRACE_SIZE));
-      trace = trace.substring(0, MAX_TRACE_SIZE) + "\n";
+      trace = failure.getTrimmedTrace();


unfortunately it looks like getTrimmedTrace is only available in JUnit 4.13. Androidx.test still uses junit 4.12.

Ah. Should I remove it? Add a reflection check to see if the method is available?

Since this is a failure at the end of a test, the performance penalty shouldn't matter. Your call!

Please remove for now, but add a TODO to use it.

I have always wanted to see stack traces trimmed and I'm stoked to see such a method available, but JUnit upgrades can be onerous , so let's pursue that separately.

Updated branch with just the value change.

googlebot added the cla: no label Aug 7, 2020

brettchabot reviewed Aug 25, 2020

View reviewed changes

pyricau force-pushed the patch-2 branch from 6af8cf5 to e1e397c Compare August 30, 2020 16:55

googlebot added cla: yes and removed cla: no labels Aug 30, 2020

brettchabot reviewed Aug 31, 2020

View reviewed changes

Increase InstrumentationResultPrinter.MAX_TRACE_SIZE

4f37f6b

pyricau force-pushed the patch-2 branch from e1e397c to 4f37f6b Compare September 2, 2020 10:00

copybara-service bot merged commit 39788c7 into android:master Sep 2, 2020

bohsen mentioned this pull request Sep 3, 2020

Trim and truncate test failure stack traces for both orchestrator and classic/non-orchestrator modes. #730

Merged

pyricau deleted the patch-2 branch September 3, 2020 06:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase InstrumentationResultPrinter.MAX_TRACE_SIZE #711

Increase InstrumentationResultPrinter.MAX_TRACE_SIZE #711

pyricau commented Aug 7, 2020

googlebot commented Aug 7, 2020

pyricau commented Aug 7, 2020

bohsen commented Aug 25, 2020

brettchabot Aug 25, 2020

bohsen Aug 25, 2020

brettchabot Aug 25, 2020

pyricau Aug 26, 2020

brettchabot Aug 26, 2020

TWiStErRob Aug 26, 2020

bohsen Aug 27, 2020 •

edited

Loading

brettchabot Aug 27, 2020

brettchabot commented Aug 27, 2020

googlebot commented Aug 30, 2020

pyricau commented Aug 30, 2020

brettchabot Aug 31, 2020

pyricau Sep 1, 2020

pyricau Sep 1, 2020

brettchabot Sep 1, 2020

pyricau Sep 2, 2020

pyricau Sep 2, 2020

		@@ -45,7 +45,7 @@

		private static final String TAG = "InstrumentationResultPrinter";

		@VisibleForTesting static final int MAX_TRACE_SIZE = 32 * 1024;

Increase InstrumentationResultPrinter.MAX_TRACE_SIZE #711

Increase InstrumentationResultPrinter.MAX_TRACE_SIZE #711

Conversation

pyricau commented Aug 7, 2020

googlebot commented Aug 7, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

pyricau commented Aug 7, 2020

bohsen commented Aug 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bohsen Aug 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brettchabot commented Aug 27, 2020

googlebot commented Aug 30, 2020

pyricau commented Aug 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bohsen Aug 27, 2020 •

edited

Loading