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

Validate console.count IDL against implementations #88

Closed
domfarolino opened this issue Mar 2, 2017 · 13 comments
Closed

Validate console.count IDL against implementations #88

domfarolino opened this issue Mar 2, 2017 · 13 comments

Comments

@domfarolino
Copy link
Member

domfarolino commented Mar 2, 2017

This is an extension of #26 where @domenic and I discussed implementation differences regarding console.time/timeEnd.


count (IDL)

With count there are two things to test (similarly to time):

  1. The implementation of the IDL's default parameter label = ""
    • This involves ensuring that console.count() and console.count(undefined) not only work, but also produce identical results
  2. ES ToString() conversion is being delegated to
    • This involves ensuring that label.toString() gets called if the count label is an object.

Tests

  • Test for (1) is in this bin
  • Test for (2) is in this bin

Results:

  • Firefox:
    • Fails (1) in that count() behaves differently than count(undefined).
      • count() uses label "no label" instead of "default"
      • count(undefined) uses label "undefined" instead of "default"
    • Fails in that errors are never propagated to the user (2)
      • ✅ Calls label.toString() when label is object.
      • ❌ Does not re-throw conversion error to user AND continues with count() behavior
  • Chrome + Canary:
    • Fails (1) in that count() behaves differently than count(undefined).
      • count() uses label "" instead of "default"
      • count(undefined) uses label "undefined" instead of "default"
    • Fails both parts of (2)
      • ❌ Does not call label.toString() ever (just like time)
      • ❌ Obviously never re-throws errors since custom conversion never takes place
      • This actually is good, because fixing time may incidentally fix count!
  • Edge:
    • Passes (1).
      • count() uses label "" instead of "default"
    • Fails in that errors are never propagated to the user (2)
      • ✅ Calls label.toString() when label is object.
      • ❌ Does not re-throw conversion error to user AND continues with count() behavior
  • Safari + Tech Preview (24):
    • Fails (1) in that count() behaves differently than count(undefined)
      • count() uses label "Global" instead of "default"
      • count(undefined) uses label "undefined" instead of "default"
    • Fails (2) (behaves just like Firefox)
      • ✅ Calls label.toString() when label is object.
      • ❌ Does not re-throw conversion error to user AND continues with count() behavior

I think this is quite a bit worse than time, however a lot of the non-compliant behavior revolves around differing default labels on count() and the fact that count(undefined) converted undefined => "undefined" as a string. Does it make sense to do something similar to what the recent changes to assert are doing, in that we can make the label optional with no default value, and in the algorithm suggest a default value but allow implementations to choose what they see fit. This may help compliancy out a bit, but we'll still need to open bugs for undefined => "undefined".

Regarding web platform tests for this, are we limited to visual tests since we cannot (to my knowledge) programmatically check to see what value a console did/didn't log after some call?

@JosephPecoraro
Copy link

Safari + Tech Preview (24):
❌ count() uses label "Global" instead of ""

What Safari is doing here is not the same as if the user used the string "Global". We are using an internal name and choosing to display "Global" because we feel it is more user friendly than an empty string.


That said, based off of Edge's implementation and the lessons learned with console.time without arguments I suggested new behavior for console.count() with no arguments in #60 which would be of interest here.

@domfarolino
Copy link
Member Author

@JosephPecoraro Ah sorry I didn't see that issue. So it seems that issue thread ended with the opinion of going with optional label = "default", such that all of the following calls would increment a single default counter:

  • console.count()
  • console.count("default")
  • console.count(undefined)

...as well as the addition of the countReset() function which identifies an internal counter given some label in the exact same manner as count does?


Let me know if I missed something. With all that said I'm concerned about the failures above. Right now only one implementation (chrome) follows default param of = "", so if we changed it to = "default" we'd technically have zero compliant implementations. Of course, we could consult implementors before actually changing the spec.

On the other hand, given the failures/inconsistencies, I wonder if it would instead make sense to loosen the IDL to void count(optional DOMString label); so that if a label isn't provided, we can write text in the spec delegating the name of the default label to the implementation. Consider:

  1. If given label is undefined, set label equal to an implementation defined title referring to the default counter.
  2. Let called be the number of times count has been invoked (including this invocation) with label.
  3. Let concat be the concatenation of label, U+003A COLON (:), U+0020 (SPACE), and ToString(called).
  4. Perform Logger("log", concat).

How do you feel about this? I'm not sure if the way I perform an undefined check in (1) is ideal or not

@domenic
Copy link
Member

domenic commented Mar 3, 2017

Edge seems to output :4 (i.e., empty string label was counted 4 times) for the first case, and it does not re-throw, but does call the custom toString.

I agree this situation seems a lot worse than time(). However we can be optimistic and say that just gives us lots of leeway to spec what's best :).

As part of that I do think settling everyone on a single label is better than leaving it up to the implementation to vary between "", "Global" (kind of), and "default".

Since @JosephPecoraro (WebKit) seems on board I would suggest going for optional DOMString label = "default".

Regarding web platform tests for this, are we limited to visual tests since we cannot (to my knowledge) programmatically check to see what value a console did/didn't log after some call?

Yes, we can add tests for the error-throwing case, but no automatic tests for the actual visual case. Manual tests are still valuable though. They are usually suffixed with -manual.html and then contain instructions like <p>Open your console. It should contain one line whose contents are <code>default: 4</code></p>.

This was referenced Mar 3, 2017
@domfarolino
Copy link
Member Author

Sounds good, I can PR optional DOMString label = "default" if we're set with that and @terinjokes @robertkowalski are on board. Will file necessary bugs too.

Regarding the default parameter case, I assume it's probably best to wait until #90 is resolved and the spec is crystal clear on how count() should be displayed before making any manual web-platform-tests/bugs. Agree?

@domenic
Copy link
Member

domenic commented Mar 6, 2017

Regarding the default parameter case, I assume it's probably best to wait until #90 is resolved and the spec is crystal clear on how count() should be displayed before making any manual web-platform-tests/bugs. Agree?

I think that's a reasonable way to do things, although I also think filing browser bugs to say "please use the label default, not a unique label per call" or similar is OK.

domfarolino added a commit to domfarolino/console that referenced this issue Mar 7, 2017
@bakulf
Copy link

bakulf commented Apr 18, 2017

Question: is the counting per script or is it global?

In firefox, the counting is per script. This means that, if we have 2 scripts foo.js and bar.js running 'in parallel' and doing: console.count('a'); something(); console.count('a') the result will be 2 - 2 and not 4.

I would like to have this feature covered in the spec. Is it already like this? Do we care?

@bakulf
Copy link

bakulf commented Apr 18, 2017

Sorry, I'm wrong. In case the label is not passed as argument, we force it to be <scriptName>:<fileName>. I'm OK to have the behavior as described in this issue.

@domenic
Copy link
Member

domenic commented Apr 18, 2017

Wait, I'm confused; are you wrong about it being per-script in Firefox, or are you wrong about something involving the label?

Per spec right now it's per-global in a vague way, although we should probably formalize it more by adding e.g. a console count map to each console instance, like we do for the group stack concept.

@domfarolino
Copy link
Member Author

domfarolino commented Apr 18, 2017

Seems wrong at least about per-script in Firefox, as the following code yields a single global counter incremented 6 times:

<!doctype HTML>
<html>
    <head></head>
    <body>
        <script src="cons1.js"></script>
        <script>
            console.count('a');
            console.count('a');
        </script>
        <script src="cons1.js"></script>
    </body>
</html>

..where cons1.js and cons2.js both have the following code

console.count('a');
console.count('a');

Regarding console.count() (no-label) case (same code otherwise) in both Firefox and Chrome I get

: 1
: 1
: 1
: 1
: 1
: 1
<no label>: 1
<no label>: 1
<no label>: 1
<no label>: 1
<no label>: 1
<no label>: 1

Which seems a little odd?

@domenic
Copy link
Member

domenic commented Apr 18, 2017

I think we knew that odd behavior would happen, and that's why you filed all the bugs :)

@domfarolino
Copy link
Member Author

Yeah, hopefully when default labels get fixed for these implementations, anomalies like this will mostly disappear. Anyways, I like the idea of a count map for sure. More rigorous.

@bakulf
Copy link

bakulf commented May 3, 2017

Currently Firefox Nightly supports the default labels for console.count(), console.time() and console.timeEnd().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants