Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix error tooltip(#83) #85

Closed
wants to merge 1 commit into from

2 participants

@andreieftimie
Collaborator

This fixes the problem we were having showing some errors.

In case of an exception, if we receive an array, we'll join the array with a
and send that forward to the template.

In the template we would draw a different "tooltip" for each "stack" message. This never worked correctly as they would overlap eachother. I've changed this to only have 1 tooltip per error, and show the stack inside.

In case of a failure we jsonify the result so this works nicely. Further down the line we might want to improve the look of this JSON object by also prettifying it. But thats another issue.

I've also improved and cleaned up the CSS a bit for the tooltip, and put up a monospaced font to make it look more like a console output.

Demo: http://mozmill-sandbox.blargon7.com/#/remote/report/40742e0746fd767e6d2fd58659a6dd52

@whimboo please review

@whimboo
Owner

First look shows me that it works fine. But one thing which is still annoying is the placement of the tooltip. When you have scrolled the page to the bottom, the tooltip is rendered mostly outside of the visible area. Is there an easy way to get this fixed?

@whimboo whimboo commented on the diff
_attachments/js/dashboard.js
@@ -90,6 +90,9 @@ function processTestResults(aReport) {
// An exception has been thrown
message = failure.exception.message;
stack = failure.exception.stack;
+ if (stack instanceof Array) {
@whimboo Owner
whimboo added a note

Can you please give an example when this happens? I don't understand why the stack can be an array.

@whimboo Owner
whimboo added a note

We usually only have multiple stacks in case of soft assertions (except) but not assert, which is this case.

@whimboo Owner
whimboo added a note

Any explanation?

@andreieftimie Collaborator

Here is an Array generated from a simple assert.fail('Custom fail'); call:
http://mozmill-sandbox.blargon7.com/#/functional/report/d8c9b09561f242bc8489be43d6433dea

This is the main reason we can't see errors right now.
Since a stack should be a JSON object, this is probably an issue in Mozmill, I'll file an issue.

@whimboo Owner
whimboo added a note

Hm, it might not be a bug in Mozmill. I think that's even a good thing that Mozmill already expands the stack as an array. I totally missed that.

I wouldn't squash it down to a string here but handle the individual entries in the template. That way we could add links to the source code later, which this join() would prevent us from.

@andreieftimie Collaborator

The problem is that in the template we would have to handle both an Array and a JSON object.
And the template is not great at handling this.

I'll try to see how we can handle this at the template level.

@andreieftimie Collaborator

I've updated the PR with another commit. We're now passing the stack as an array to the template. To handle both the JSON obj and the Array I wrapped the Array in another object.

Otherwise the template would iterate over all Array items.

@whimboo Owner
whimboo added a note

Can you please push those changes to sandbox, and make sure we have each of those conditions in the reports:

  • pass for expect
  • pass for assert
  • fail for expect
  • fail for assert
@andreieftimie Collaborator

Here is a report that has fails for both expect and assert.
(It also has passes).
http://mozmill-sandbox.blargon7.com/#/functional/report/16dc29d7a411d565992bbd7dc84ee26d

I have pushed another commit to properly indent the JSON output. Looks much better now.

Given these rather large error stacks, I'm getting in favor of not limiting in size the tooltip.
When we want to see details, it might be better to have it all displayed in one big box. Otherwise you always need to copy it into a local texteditor. And this is a bit cumbersome. Not sure if a textarea might not be better for this purpose (easy copy/paste)

@whimboo Owner
whimboo added a note

We can do that as a follow-up patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@andreieftimie
Collaborator

The tooltip itself also bothers me.
I would actually move it out from being a tooltip and instead show it in the document directly.

It would be hidden initially and it would show when clicking on a "See error details" button of some sort.
Does that sound reasonable?

@whimboo
Owner

Sounds like a good idea, yes!

@andreieftimie
Collaborator

I've updated this PR since we're still having problems seeing some error messages.

Here's a sample:
http://mozmill-sandbox.blargon7.com/#/functional/report/e35f22a68fec3f6d144713f9aba62e5b

There's a show more link that toggles the detail of the error which is now displayed inline.

@whimboo
Owner

Thanks for the update Andrei. It looks good. Some things I have noticed:

  • When you click on show more the contents should change to show less.
  • Make sure to not widen the table when clicking on show more.
  • Not sure why but the stack contains some unwanted entries like frame.js ->. Could be a Mozmill issue. Usually we should filter those out.
@whimboo
Owner

Also please do not squash commits between reviews. That makes it harder to see the differences.

_attachments/css/main.css
((21 lines not shown))
border: 1px solid #333;
- -moz-border-radius: 10px;
@whimboo Owner
whimboo added a note

I would keep the radius but make it smaller. Together with a bit of blurring like the text boxes here on github would look great.

@andreieftimie Collaborator

Should be closer to what you envisioned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@whimboo whimboo commented on the diff
_attachments/js/dashboard.js
@@ -2274,3 +2279,17 @@ function processTestResults(aReport) {
});
})(jQuery);
+
+/**
+ * Toggle error tooltips
+ */
+(function($) {
+ $(document).bind('click', function (e) {
+ var target = $(e.target);
+ if (target.is('.show-tooltip')) {
+ target.next('.tooltip').toggleClass('active');
+ e.preventDefault();
+ return false;
+ }
+ });
+})(jQuery);
@whimboo Owner
whimboo added a note

Why a separate function here and not making it part of the general jQuery one?

@andreieftimie Collaborator

Modularity...

Tha main jQuery DOM Ready callback function handles:

  1. the sammy app
  2. invokes the sammy app
  3. has a request helper function

(I feel even this should have been separated, but that is another discussion)

I could move this together with the rest, but it's better (for modularity, resiliency, separation of concerns) to have many smaller blocks.

@whimboo Owner
whimboo added a note

I think that this has only to be applied to the report type. As of now you would run it against any kind of page.

@andreieftimie Collaborator

We are adding this to all pages as we don't have real separate pages.
And there's no clear indication when we are on a report page.

There is less overhead in adding this globally then always adding & removing an event handler on each page-change. If we would have better separation in the app maybe it would be easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
_attachments/js/dashboard.js
@@ -106,7 +109,9 @@ function processTestResults(aReport) {
// A plain JS error has been reported
message = failure.message;
}
-
+ if (stack) {
+ stack = '<a href="#" class="show-tooltip">show more</a><div class="tooltip">' + stack + '</div>';
@whimboo Owner
whimboo added a note

Can we please keep the structure in the template?

@andreieftimie Collaborator

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
_attachments/css/main.css
((8 lines not shown))
table .information {
vertical-align: top;
margin: 0;
padding: 0 10px;
}
-table#result tr.failed .information:hover .tooltip {
- position: absolute;
- display: block;
+.tooltip {
+ background: rgb(243, 243, 243);
+ border: 1px solid rgb(216, 213, 213);
+ border-radius: 4px;
+ box-shadow: 0 0 5px 0 rgb(191, 189, 189);
+ display: none;
+ font-size: 1.2em;
+ font-family: monospace;
+ margin: 1em 0;
+ max-width: 35em;
@whimboo Owner
whimboo added a note

Lovely that box! Just one more thing. Can we make that the box has the same padding on the right side in the cell? That would give a bit more space for the content to be shown without having to scroll.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@andreieftimie
Collaborator

I added some padding to the right side of the tooltip as well.
I also squashed all commits (sorry @whimboo for this, I know you don't like it, but for some reason I couldn't push to this branch without force, with this in mind, and because we're probably close to merging I also squashed them)

_attachments/js/dashboard.js
@@ -94,19 +94,24 @@ function processTestResults(aReport) {
// An exception has been thrown
message = failure.exception.message;
stack = failure.exception.stack;
+ if (stack instanceof Array) {
+ stack = stack.map(function (item, i) {
+ return ((i === 0) ? "" : "\n") + item;
+ });
@whimboo Owner
whimboo added a note

Why cannot we do a '\n'.join(stack) here? Also please indent by 2 chars and not somewhat 20.

@whimboo Owner
whimboo added a note

Ups. We are not in Python land, so stack.join("\n").

@whimboo Owner
whimboo added a note

Oh wait. We want to retain the array. So it looks fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@whimboo
Owner

@andreieftimie that looks fine now! Lets get this squashed and merged to master. Can you do that, and pushing the doc changes to all dashboards?

@andreieftimie
Collaborator

Merged in ae9d0e2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 21, 2014
  1. @andreieftimie

    Fix error tooltip(#83)

    andreieftimie authored
This page is out of date. Refresh to see the latest.
View
40 _attachments/css/main.css
@@ -106,7 +106,6 @@ html ul.topnav li ul.subnav li a:hover {
}
#content {
- width: 90%;
margin: 0 auto;
padding-top: 10px;
clear: both;
@@ -230,6 +229,10 @@ table#graphics {
margin: 10px 0 0 0;
}
+#result td:last-child {
+ min-width: 38em;
+}
+
table#results td, table#results th {
margin: 0;
}
@@ -264,25 +267,36 @@ table#result tr.skipped td {
color: #f90;
}
-.tooltip {
- display: none;
-}
-
table .information {
vertical-align: top;
margin: 0;
padding: 0 10px;
}
-table#result tr.failed .information:hover .tooltip {
- position: absolute;
+.tooltip {
+ background: rgb(243, 243, 243);
+ border: 1px solid rgb(216, 213, 213);
+ border-radius: 4px;
+ box-shadow: 0 0 5px 0 rgb(191, 189, 189);
+ display: none;
+ font-size: 1.2em;
+ font-family: monospace;
+ margin: 1em 0;
+ padding: .5em 1em;
+ white-space: pre;
+}
+
+.tooltip div {
+ overflow: auto;
+ max-width: 35em;
+ max-height: 30em;
+}
+
+.tooltip.active {
display: block;
- overflow: scroll;
- width: 400px;
- border: 1px solid #333;
- -moz-border-radius: 10px;
- padding: 5px 10px;
- background-color: #fff;
+}
+.show-tooltip {
+ font-size: 0.8em;
}
th.headerSortUp {
View
25 _attachments/js/dashboard.js
@@ -94,19 +94,24 @@ function processTestResults(aReport) {
// An exception has been thrown
message = failure.exception.message;
stack = failure.exception.stack;
+ if (stack instanceof Array) {
@whimboo Owner
whimboo added a note

Can you please give an example when this happens? I don't understand why the stack can be an array.

@whimboo Owner
whimboo added a note

We usually only have multiple stacks in case of soft assertions (except) but not assert, which is this case.

@whimboo Owner
whimboo added a note

Any explanation?

@andreieftimie Collaborator

Here is an Array generated from a simple assert.fail('Custom fail'); call:
http://mozmill-sandbox.blargon7.com/#/functional/report/d8c9b09561f242bc8489be43d6433dea

This is the main reason we can't see errors right now.
Since a stack should be a JSON object, this is probably an issue in Mozmill, I'll file an issue.

@whimboo Owner
whimboo added a note

Hm, it might not be a bug in Mozmill. I think that's even a good thing that Mozmill already expands the stack as an array. I totally missed that.

I wouldn't squash it down to a string here but handle the individual entries in the template. That way we could add links to the source code later, which this join() would prevent us from.

@andreieftimie Collaborator

The problem is that in the template we would have to handle both an Array and a JSON object.
And the template is not great at handling this.

I'll try to see how we can handle this at the template level.

@andreieftimie Collaborator

I've updated the PR with another commit. We're now passing the stack as an array to the template. To handle both the JSON obj and the Array I wrapped the Array in another object.

Otherwise the template would iterate over all Array items.

@whimboo Owner
whimboo added a note

Can you please push those changes to sandbox, and make sure we have each of those conditions in the reports:

  • pass for expect
  • pass for assert
  • fail for expect
  • fail for assert
@andreieftimie Collaborator

Here is a report that has fails for both expect and assert.
(It also has passes).
http://mozmill-sandbox.blargon7.com/#/functional/report/16dc29d7a411d565992bbd7dc84ee26d

I have pushed another commit to properly indent the JSON output. Looks much better now.

Given these rather large error stacks, I'm getting in favor of not limiting in size the tooltip.
When we want to see details, it might be better to have it all displayed in one big box. Otherwise you always need to copy it into a local texteditor. And this is a bit cumbersome. Not sure if a textarea might not be better for this purpose (easy copy/paste)

@whimboo Owner
whimboo added a note

We can do that as a follow-up patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ stack = stack.map(function (item, i) {
+ return ((i === 0) ? "" : "\n") + item;
+ });
+ stack = { stack: stack };
+ }
}
else if ("fail" in failure) {
// An assertion failed
message = failure.fail.message;
if ("stack" in failure.fail) {
- stack = JSON.stringify(failure.fail.stack);
+ stack = JSON.stringify(failure.fail.stack, null, " ");
}
}
else if ("message" in failure) {
// A plain JS error has been reported
message = failure.message;
}
-
info.push({message: message, stack: stack});
}
}
@@ -2274,3 +2279,19 @@ function processTestResults(aReport) {
});
})(jQuery);
+
+/**
+ * Toggle error tooltips
+ */
+(function($) {
+ $(document).bind('click', function (e) {
+ var target = $(e.target);
+ if (target.is('.show-tooltip')) {
+ var tooltip = target.next('.tooltip');
+ tooltip.toggleClass('active');
+ target.text(tooltip.is('.active') ? 'show less' : 'show more');
+ e.preventDefault();
+ return false;
+ }
+ });
+})(jQuery);
@whimboo Owner
whimboo added a note

Why a separate function here and not making it part of the general jQuery one?

@andreieftimie Collaborator

Modularity...

Tha main jQuery DOM Ready callback function handles:

  1. the sammy app
  2. invokes the sammy app
  3. has a request helper function

(I feel even this should have been separated, but that is another discussion)

I could move this together with the rest, but it's better (for modularity, resiliency, separation of concerns) to have many smaller blocks.

@whimboo Owner
whimboo added a note

I think that this has only to be applied to the report type. As of now you would run it against any kind of page.

@andreieftimie Collaborator

We are adding this to all pages as we don't have real separate pages.
And there's no clear indication when we are on a report page.

There is less overhead in adding this globally then always adding & removing an event handler on each page-change. If we would have better separation in the app maybe it would be easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
View
6 _attachments/templates/addons_report.mustache
@@ -100,7 +100,10 @@
{{#info}}
<li>
{{{message}}}
- {{#stack}}<div class="tooltip">{{{stack}}}<div>{{/stack}}
+ {{#stack}}
+ <a href="#" class="show-tooltip">show more</a>
+ <div class="tooltip"><div>{{{stack}}}</div></div>
+ {{/stack}}
</li>
{{/info}}
</ul>
@@ -110,4 +113,3 @@
</tbody>
</table>
</body>
-
View
6 _attachments/templates/endurance_report.mustache
@@ -303,7 +303,10 @@
{{#info}}
<li>
{{{message}}}
- {{#stack}}<div class="tooltip">{{{stack}}}<div>{{/stack}}
+ {{#stack}}
+ <a href="#" class="show-tooltip">show more</a>
+ <div class="tooltip"><div>{{{stack}}}</div></div>
+ {{/stack}}
</li>
{{/info}}
</ul>
@@ -314,4 +317,3 @@
</table>
</body>
-
View
6 _attachments/templates/functional_report.mustache
@@ -84,7 +84,10 @@
{{#info}}
<li>
{{{message}}}
- {{#stack}}<div class="tooltip">{{{stack}}}<div>{{/stack}}
+ {{#stack}}
+ <a href="#" class="show-tooltip">show more</a>
+ <div class="tooltip"><div>{{{stack}}}</div></div>
+ {{/stack}}
</li>
{{/info}}
</ul>
@@ -94,4 +97,3 @@
</tbody>
</table>
</body>
-
View
5 _attachments/templates/l10n_report.mustache
@@ -84,7 +84,10 @@
{{#info}}
<li>
{{{message}}}
- {{#stack}}<div class="tooltip">{{{stack}}}<div>{{/stack}}
+ {{#stack}}
+ <a href="#" class="show-tooltip">show more</a>
+ <div class="tooltip"><div>{{{stack}}}</div></div>
+ {{/stack}}
</li>
{{/info}}
</ul>
View
6 _attachments/templates/remote_report.mustache
@@ -84,7 +84,10 @@
{{#info}}
<li>
{{{message}}}
- {{#stack}}<div class="tooltip">{{{stack}}}<div>{{/stack}}
+ {{#stack}}
+ <a href="#" class="show-tooltip">show more</a>
+ <div class="tooltip"><div>{{{stack}}}</div></div>
+ {{/stack}}
</li>
{{/info}}
</ul>
@@ -94,4 +97,3 @@
</tbody>
</table>
</body>
-
View
6 _attachments/templates/update_report.mustache
@@ -153,7 +153,10 @@
{{#info}}
<li>
{{{message}}}
- {{#stack}}<div class="tooltip">{{{stack}}}<div>{{/stack}}
+ {{#stack}}
+ <a href="#" class="show-tooltip">show more</a>
+ <div class="tooltip"><div>{{{stack}}}</div></div>
+ {{/stack}}
</li>
{{/info}}
</ul>
@@ -163,4 +166,3 @@
</tbody>
</table>
</body>
-
Something went wrong with that request. Please try again.