Closed Bug 760193 Opened 12 years ago Closed 11 years ago

Add console.assert

Categories

(DevTools :: Console, defect, P3)

Product:
DevTools
Component:
Console
Version:
14 Branch
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 28

People

(Reporter: dangoor, Assigned: denschub)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

Proposed implementation of console.assert()
13.20 KB, patch
Details | Diff | Splinter Review 
The Firefox console should have the assert method, which is part of the de facto standard.

http://getfirebug.com/wiki/index.php/Console_API#console.assert.28expression.5B.2C_object.2C_....5D.29
Priority: -- → P3
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All

        Attachment #8344001 -
        Flags: review?(mihai.sucan)

    
    

    
  
Comment on attachment 8344001 [details] [diff] [review]
Proposed implementation of console.assert()

Review of attachment 8344001 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking good. Thank you for the patch! Really good work!

r+ with the comments below addressed.

::: browser/devtools/webconsole/test/browser.ini
@@ +1,4 @@
>  [DEFAULT]
>  support-files =
>    head.js
> +  test-assert.html

test-console-assert.html

::: browser/devtools/webconsole/test/test-assert.html
@@ +5,5 @@
> +    <script type="text/javascript">
> +      function test() {
> +        console.log("start");
> +        console.assert(false, "false assert");
> +        console.assert(true, "true assert");

Add a test for a falsy value as well.

::: dom/base/ConsoleAPI.js
@@ +133,5 @@
>                                       null);  
>        },
> +      assert: function CA_assert() {
> +        let args = Array.prototype.slice.call(arguments);
> +        if(args.shift() === false) {

assert not only for === false, any falsy value.

if (!args.shift()) { ...

        Attachment #8344001 -
        Flags: review?(mihai.sucan) → review+

    
    

    
  


  
Thank you very much for your express review, Mihai! Also thanks for your feedback, addressed in the patch attached.

        Attachment #8344001 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8344179 [details] [diff] [review]
Proposed implementation of console.assert()

Review of attachment 8344179 [details] [diff] [review]:
-----------------------------------------------------------------

The try push was green. Thanks!

Before landing, more comments:

::: browser/devtools/webconsole/test/browser_webconsole_assert.js
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const TEST_URI = "http://example.com/browser/browser/devtools/webconsole/test/test-console-assert.html";

Please add a short comment describing the test. Something like:

  // Test that console.assert() works as expected. See bug 760193.

@@ +21,5 @@
> +      category: CATEGORY_WEBDEV,
> +      severity: SEVERITY_LOG,
> +    },
> +    {
> +      text: "false assert",

add the "falsy assert" message as well.

::: browser/devtools/webconsole/test/test-console-assert.html
@@ +1,3 @@
> +<!DOCTYPE HTML>
> +<html dir="ltr" xml:lang="en-US" lang="en-US"><head>
> +    <meta charset="utf-8">

Please add the PD license boilerplate.

https://www.mozilla.org/MPL/headers/

    
    

    
  
Also please use the PD license boilerplate in the new browser_webconsole_assert.js file, instead of MPL. We use PD for tests.

    
    

    
  


  
Here we are. :)

        Attachment #8344179 -
        Attachment is obsolete: true

    
    

    
  
Danke schön!

Landed: https://hg.mozilla.org/integration/fx-team/rev/aa759ea2238d
Whiteboard: [fixed-in-fx-team]
Keywords: dev-doc-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/aa759ea2238d
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
Blocks: 967226

    
    

    
  
Mihai: I've added this to the Web Console docs, and generally extended the documetation on logging while I was in there: https://developer.mozilla.org/en-US/docs/Tools/Web_Console#console_API_messages. Please let me know if it works for you.

It's a problem that the table of contents overlaps the table of console API calls slightly: I could not figure out how to make the console output narrow enough so this does not happen. I'll take another look tomorrow.

Will
Flags: needinfo?(mihai.sucan)

    
    

    
  
(In reply to Will Bamberg [:wbamberg] from comment #10)
> Mihai: I've added this to the Web Console docs, and generally extended the
> documetation on logging while I was in there:
> https://developer.mozilla.org/en-US/docs/Tools/
> Web_Console#console_API_messages. Please let me know if it works for you.

Thanks for the updates! The docs look good. Comments:

- console.error(), you might want to mention exception(), which was added as an alias, in Firefox 28 - bug 922214.
- Should the list be exhaustive? It doesn't include debug() and profile*().
- group*() stuff should be added in the table to make it more obvious when skimming through the list. The final note after the table, which mentions group() functions, is not obvious.

Went ahead and I also checked:

- https://developer.mozilla.org/en-US/docs/Web/API/console#Stack_traces

This should be updated with the new graphics for console.trace().

This page also links to:

- https://developer.mozilla.org/en-US/docs/Trash/Using_the_Remote_Web_Console

This article is marked as needing a technical review, and it's in /Trash/. Review: the article is good, but seriously outdated. Should be updated to refer people to the app manager, which allows users to debug apps with the whole toolbox, including the console. All in a much nicer UI/UX. Or delete the article entirely, and any links to it.


> It's a problem that the table of contents overlaps the table of console API
> calls slightly: I could not figure out how to make the console output narrow
> enough so this does not happen. I'll take another look tomorrow.

The table looks good now.

Thank you!
Flags: needinfo?(mihai.sucan)

    
    

    
  
isn't this method supposed to throw if the assertion fails, according to the specification linked in the bug report?

http://getfirebug.com/wiki/index.php/Console_API#console.assert.28expression.5B.2C_object.2C_....5D.29
Flags: needinfo?(mozilla)
Flags: needinfo?(mihai.sucan)

    
    

    
  
According to the spec on GitHub (https://github.com/DeveloperToolsWG/console-object/blob/master/api.md#consoleassertexpression-object):

> If the specified expression is false, the message is written to the console
> along with a stack trace.

As far as I can tell, the devtools team wants to have that spec implemented, see bug 922204. It's not printing a stack trace at the moment, but theres bug 967226 for that.
Flags: needinfo?(mozilla)

    
    

    
  
ah, didn't see the bug 96722 linked above, sorry..
Flags: needinfo?(mihai.sucan)

    
    

    
  
Thanks Mihai!

(In reply to Mihai Sucan [:msucan] from comment #11)
> (In reply to Will Bamberg [:wbamberg] from comment #10)
> > Mihai: I've added this to the Web Console docs, and generally extended the
> > documetation on logging while I was in there:
> > https://developer.mozilla.org/en-US/docs/Tools/
> > Web_Console#console_API_messages. Please let me know if it works for you.
> 
> Thanks for the updates! The docs look good. Comments:
> 
> - console.error(), you might want to mention exception(), which was added as
> an alias, in Firefox 28 - bug 922214.

I've added exception().

> - Should the list be exhaustive? It doesn't include debug() and profile*().
> - group*() stuff should be added in the table to make it more obvious when
> skimming through the list. The final note after the table, which mentions
> group() functions, is not obvious.

I omitted debug() just on the basis that it's been deprecated for years. I omitted profile*() because it doesn't produce any console output, and this table is really supposed to show console output. I omitted group*() for the same reasons (though I did just go and make group*() more obvious).

My idea was that this table just shows Web Console output. It's complementary to the console API page, but while that documents how the API works from the point of view of calling code, and hopefully in a browser-agnostic fashion, this page documents, specifically, how that looks in the Web Console in Firefox DevTools. It might look different in other browsers (or even, in different consoles in the same browser, like Firebug's console).

So while https://developer.mozilla.org/en-US/docs/Web/API/console should document everything that might be considered de facto standard (and should summarise which APIs are available for which browsers), it should not be very particular about what the result looks like in any given console implementation. I would not expect this page to be updated when you land improvements to the Web Console's output.

But https://developer.mozilla.org/en-US/docs/Tools/Web_Console#console_API_messages is specifically about how particular console API calls appear in the Web Console. I have added a sentence at the top trying to clarify the distinction between this section and the console API documentation page.

However, I'm happy to reconsider these choices if you think they're not right. 

> 
> Went ahead and I also checked:
> 
> - https://developer.mozilla.org/en-US/docs/Web/API/console#Stack_traces
> 
> This should be updated with the new graphics for console.trace().

I have updated that graphic, but as noted above, I don't want this page to get too much into the details of any particular console implementation.

> 
> This page also links to:
> 
> - https://developer.mozilla.org/en-US/docs/Trash/Using_the_Remote_Web_Console
> 
> This article is marked as needing a technical review, and it's in /Trash/.
> Review: the article is good, but seriously outdated. Should be updated to
> refer people to the app manager, which allows users to debug apps with the
> whole toolbox, including the console. All in a much nicer UI/UX. Or delete
> the article entirely, and any links to it.

I've removed that link in https://developer.mozilla.org/en-US/docs/Web/API/console#See_also and pointed people at https://developer.mozilla.org/en-US/docs/Tools/Remote_Debugging instead, and directly to https://developer.mozilla.org/en-US/Firefox_OS/Using_the_App_Manager if they're developing apps.

    
    

    
  
(In reply to Will Bamberg [:wbamberg] from comment #15)
> Thanks Mihai!
> 
> (In reply to Mihai Sucan [:msucan] from comment #11)
> > (In reply to Will Bamberg [:wbamberg] from comment #10)
> > > Mihai: I've added this to the Web Console docs, and generally extended the
> > > documetation on logging while I was in there:
> > > https://developer.mozilla.org/en-US/docs/Tools/
> > > Web_Console#console_API_messages. Please let me know if it works for you.
> > 
> > Thanks for the updates! The docs look good. Comments:
> > 
> > - console.error(), you might want to mention exception(), which was added as
> > an alias, in Firefox 28 - bug 922214.
> 
> I've added exception().

Thanks!

> > - Should the list be exhaustive? It doesn't include debug() and profile*().
> > - group*() stuff should be added in the table to make it more obvious when
> > skimming through the list. The final note after the table, which mentions
> > group() functions, is not obvious.
> 
> I omitted debug() just on the basis that it's been deprecated for years. I
> omitted profile*() because it doesn't produce any console output, and this
> table is really supposed to show console output. I omitted group*() for the
> same reasons (though I did just go and make group*() more obvious).
> 
> My idea was that this table just shows Web Console output. It's
> complementary to the console API page, but while that documents how the API
> works from the point of view of calling code, and hopefully in a
> browser-agnostic fashion, this page documents, specifically, how that looks
> in the Web Console in Firefox DevTools. It might look different in other
> browsers (or even, in different consoles in the same browser, like Firebug's
> console).
> 
> So while https://developer.mozilla.org/en-US/docs/Web/API/console should
> document everything that might be considered de facto standard (and should
> summarise which APIs are available for which browsers), it should not be
> very particular about what the result looks like in any given console
> implementation. I would not expect this page to be updated when you land
> improvements to the Web Console's output.
> 
> But
> https://developer.mozilla.org/en-US/docs/Tools/
> Web_Console#console_API_messages is specifically about how particular
> console API calls appear in the Web Console. I have added a sentence at the
> top trying to clarify the distinction between this section and the console
> API documentation page.

Makes sense. I didn't know of this distinction between the two pages.


> > Went ahead and I also checked:
> > 
> > - https://developer.mozilla.org/en-US/docs/Web/API/console#Stack_traces
> > 
> > This should be updated with the new graphics for console.trace().
> 
> I have updated that graphic, but as noted above, I don't want this page to
> get too much into the details of any particular console implementation.

The console API page should probably not include screenshots, if the purpose of the page is to be browser-agnostic. With screenshots we add the need of keeping them up-to-date, irrespective of the page where they are used.


> > This page also links to:
> > 
> > - https://developer.mozilla.org/en-US/docs/Trash/Using_the_Remote_Web_Console
> > 
> > This article is marked as needing a technical review, and it's in /Trash/.
> > Review: the article is good, but seriously outdated. Should be updated to
> > refer people to the app manager, which allows users to debug apps with the
> > whole toolbox, including the console. All in a much nicer UI/UX. Or delete
> > the article entirely, and any links to it.
> 
> I've removed that link in
> https://developer.mozilla.org/en-US/docs/Web/API/console#See_also and
> pointed people at
> https://developer.mozilla.org/en-US/docs/Tools/Remote_Debugging instead, and
> directly to
> https://developer.mozilla.org/en-US/Firefox_OS/Using_the_App_Manager if
> they're developing apps.

Great! Thanks for the updates.

    
  
Product: Firefox → DevTools

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: