Style guide for layout test files

[Takeshi Yoshino]

IIRC, there's no official style guide document for Blink layout test files now.

I like kinda freedom of the layout tests directory, but sometimes feels confused for different style preference from different reviewers and wonders what's the justification for rules that I'm following or a reviewer asks me to follow.

I've created a place to gather such information about layout test file styling. Please feel free to fill it or discuss on this thread.

https://sites.google.com/a/chromium.org/dev/blink/coding-style/layout-test-style-guidelines

[Dominic Cooney]

FWIW Service Worker writes two separate styles of layout tests: ones that check spec requirements; and ones that check Blink idiosyncrasies like garbage collection and not-crashing.

For the first kind we are adopting the style in the W3C testharness.js file. We are going to upstream these files to the W3C and delete our local copy (and have them automanually reimported) which is why we chose to vary the style. Unfortunately we were a bit ambivalent at first (also about Promises; there wasn't a strong precedent for testing with them) so we have some clean-up to do. We mix the two kinds of files in the same directory because it is easy to know which is which by the style, and if necessary by grepping for the inclusion of testharness.js.

This is part of a larger strategy to split responsibilities between specing and implementing so that each blocks on the other less, and use the W3C tests as a way to reconcile the two.

I would hesitate to recommend this to anyone until our experiment is over but I'm happy to discuss whether this is a good, indifferent or terrible idea.

Dominic

[Mounir Lamouri]

As far as I am concerned, js-tests has a lot of issues that testharness
does not have. Is there any advantage in using js-tests over
testharness? I think Dominic's use case is fair but maybe we could solve
it in ways than doesn't require two tests frameworks?

-- Mounir

> To unsubscribe from this group and stop receiving emails from it, send an
> email to blink-dev+...@chromium.org.

[Dominic Cooney]

In my experience testharness.js is nicer than js-tests because the way we have it set up (thanks to some unnamed hero... Dirk?) you don't need expectation files for success.

The downside is that you have less visibility that your tests actually ran. On the whole I like the way we have testharness.js set up. It would not be crazy for someone to write all of their tests using testharness.js.

I think we will have >1 test harnesses for some time unless we have a big LayoutTest fixit.

Dominic

[Zhiqiang Zhang]

On Wed, Jul 2, 2014 at 1:37 PM, Dominic Cooney <domi...@chromium.org> wrote:

In my experience testharness.js is nicer than js-tests because the way we have it set up (thanks to some unnamed hero... Dirk?) you don't need expectation files for success.

The downside is that you have less visibility that your tests actually ran. On the whole I like the way we have testharness.js set up. It would not be crazy for someone to write all of their tests using testharness.js.

I think we will have >1 test harnesses for some time unless we have a big LayoutTest fixit.

​+1 for the testharness.js ​

​

​tests.​

--

Thanks,

Zhiqiang

[Mounir Lamouri]

On Wed, 2 Jul 2014, at 15:37, Dominic Cooney wrote:
> The downside is that you have less visibility that your tests actually
> ran.

What do you mean? Is it just a tooling issue? Maybe we could show
results in the terminal like we do for gtest-based suites?

> On the whole I like the way we have testharness.js set up. It would not
> be
> crazy for someone to write all of their tests using testharness.js.

I wonder if it would be crazy to entirely retire js-tests? I am trying
to understand what we would need for that to happen or why it can't
happen.

-- Mounir

[Dirk Pranke]

On Wed, Jul 2, 2014 at 2:59 AM, Mounir Lamouri <mou...@lamouri.fr> wrote:

> On the whole I like the way we have testharness.js set up. It would not

> be
> crazy for someone to write all of their tests using testharness.js.

I wonder if it would be crazy to entirely retire js-tests? I am trying
to understand what we would need for that to happen or why it can't
happen.

I think it would be a lot of work to convert the tests over, and it's unclear if the reward would be worth it.

Perhaps a better use of time (and a step along the path as well) would be to find tests that we're currently running that are old copies of the current w3c tests, and migrate to the newer ones (many of which are reftests instead of pixel tests, or have been converted from js-tests to testharness-tests).

-- Dirk

[Mounir Lamouri]

On Thu, 3 Jul 2014, at 01:45, Dirk Pranke wrote:
> On Wed, Jul 2, 2014 at 2:59 AM, Mounir Lamouri <mou...@lamouri.fr> wrote:
>
> > > On the whole I like the way we have testharness.js set up. It would not
> > > be
> > > crazy for someone to write all of their tests using testharness.js.
> >
> > I wonder if it would be crazy to entirely retire js-tests? I am trying
> > to understand what we would need for that to happen or why it can't
> > happen.
> >
>
> I think it would be a lot of work to convert the tests over, and it's
> unclear if the reward would be worth it.

Converting the old tests would likely be a time waste. I was mostly
wondering if there was a reason why we should allow more of the js-tests
to be added?

-- Mounir

[Domenic Denicola]

From: dpr...@google.com <dpr...@google.com> on behalf of Dirk Pranke <dpr...@chromium.org>

> I think it would be a lot of work to convert the tests over, and it's unclear if the reward would be worth it.

For any tests that could be contributed to the general W3C web-platform-tests suite, run by multiple browsers, I think there would be a big upside for the ecosystem, if not for Blink directly. It could be a good "beginner bug" even, or test-the-web-forward project.

I don't have enough knowledge or context to know what fraction of the js-tests that encompasses, however.

[Dirk Pranke]

If you were testing blink-specific things and needed test hooks that js-tests provide and testharness doesn't, it seems like there's not a super-compelling reason to use testharness.

Note that we don't directly control testharness (the framework comes from the w3c and is used by other browsers) and so patches to it need to be upstreamed. Obviously, we should try to upstream patches that we think are good ideas, but I suspect that won't always work.

-- Dirk

[Dominic Cooney]

On Thu, Jul 3, 2014 at 1:38 AM, Dirk Pranke <dpr...@chromium.org> wrote:

On Wed, Jul 2, 2014 at 9:17 AM, Mounir Lamouri <mou...@lamouri.fr> wrote:

On Thu, 3 Jul 2014, at 01:45, Dirk Pranke wrote:
> On Wed, Jul 2, 2014 at 2:59 AM, Mounir Lamouri <mou...@lamouri.fr> wrote:
>
> > > On the whole I like the way we have testharness.js set up. It would not
> > > be
> > > crazy for someone to write all of their tests using testharness.js.
> >
> > I wonder if it would be crazy to entirely retire js-tests? I am trying
> > to understand what we would need for that to happen or why it can't
> > happen.
> >
>
> I think it would be a lot of work to convert the tests over, and it's
> unclear if the reward would be worth it.

Converting the old tests would likely be a time waste. I was mostly
wondering if there was a reason why we should allow more of the js-tests
to be added?

If you were testing blink-specific things and needed test hooks that js-tests provide and testharness doesn't, it seems like there's not a super-compelling reason to use testharness.

Most of those hooks are directly on testRunner and internals, which you can use from testharness.js.

[Dominic Cooney]

On Wed, Jul 2, 2014 at 6:59 PM, Mounir Lamouri <mou...@lamouri.fr> wrote:

On Wed, 2 Jul 2014, at 15:37, Dominic Cooney wrote:
> The downside is that you have less visibility that your tests actually
> ran.

What do you mean?

I mean we have this testharnessreport.js thing and it just prints a one line "success" message. So if you add a test, you're not certain it ran unless you have omnipotent JavaScript powers, console.log, or a failure in there.

 

Is it just a tooling issue? Maybe we could show
results in the terminal like we do for gtest-based suites?

That would be neat. I am not sure how the integration works but I imagine this is possible.

[Alan Cutter]

On 3 July 2014 14:19, Dominic Cooney <domi...@chromium.org> wrote:

On Wed, Jul 2, 2014 at 6:59 PM, Mounir Lamouri <mou...@lamouri.fr> wrote:

On Wed, 2 Jul 2014, at 15:37, Dominic Cooney wrote:
> The downside is that you have less visibility that your tests actually
> ran.

What do you mean?

I mean we have this testharnessreport.js thing and it just prints a one line "success" message. So if you add a test, you're not certain it ran unless you have omnipotent JavaScript powers, console.log, or a failure in there

One could debate that this point should be part of testing style; tests should not contain complicated logic such that programmer error could result in test paths not being executed.

[Takeshi Yoshino]

Thanks all for input. I've updated the site.

Please take a look and feel free to add new items there directly or propose at this thread.

https://sites.google.com/a/chromium.org/dev/blink/coding-style/layout-test-style-guidelines

Takeshi

[Julien Chaffraix]

On Fri, Jul 25, 2014 at 12:50 AM, Takeshi Yoshino <tyos...@chromium.org> wrote:
> Thanks all for input. I've updated the site.
>
> Please take a look and feel free to add new items there directly or propose
> at this thread.
>
> https://sites.google.com/a/chromium.org/dev/blink/coding-style/layout-test-style-guidelines

Referring to the Chromium C++ style would be better than Google C++
style IMO (even if they are close).

I also don't think that the new policy gives us much: by trying to
accommodate the status quo (which is that we have at least 4 competing
styles), it has lost any potential value apart from asking people to
be consistent. I am probably missing something but It would be
immensely better to settle on one style for internal tests and one for
external ones. I would support settling on a single style but I could
see that as a tough call in our current situation.

Julien

[Takeshi Yoshino]

On Tue, Jul 29, 2014 at 1:21 AM, Julien Chaffraix <jchaf...@chromium.org> wrote:

On Fri, Jul 25, 2014 at 12:50 AM, Takeshi Yoshino <tyos...@chromium.org> wrote:
> Thanks all for input. I've updated the site.
>
> Please take a look and feel free to add new items there directly or propose
> at this thread.
>
> https://sites.google.com/a/chromium.org/dev/blink/coding-style/layout-test-style-guidelines

Referring to the Chromium C++ style would be better than Google C++
style IMO (even if they are close).

Changed. Thanks

 

I also don't think that the new policy gives us much: by trying to
accommodate the status quo (which is that we have at least 4 competing
styles), it has lost any potential value apart from asking people to
be consistent. I am probably missing something but It would be
immensely better to settle on one style for internal tests and one for
external ones. I would support settling on a single style but I could
see that as a tough call in our current situation.

Shall we converge the indent and brace rule section into 2 choices, Blink (4 && next line) or Chromium (2 && tail)?

[Ojan Vafai]

The following sections either don't have anything to do with style or don't match the common practice that I've seen, so I'd remove them:

Comparison (=== vs. ==)

Asynchronous tests

Writing HTTP tests

Stable tests

TestExpectation family files

License boilerplate

The rest seems fine to me and matches common practice I think.

[Takeshi Yoshino]

On Tue, Jul 29, 2014 at 3:04 PM, Ojan Vafai <oj...@chromium.org> wrote:

The following sections either don't have anything to do with style or don't match the common practice that I've seen, so I'd remove them:

Comparison (=== vs. ==)

Do you mean that this is not about style but just general JS common practice?

 

Asynchronous tests

Just removed

 

Writing HTTP tests

Moved to https://sites.google.com/a/chromium.org/dev/developers/testing/webkit-layout-tests?pli=1

 

Stable tests

Moved to https://sites.google.com/a/chromium.org/dev/blink/runtime-enabled-features

 

TestExpectation family files

No contents. Removed.

 

License boilerplate

What's your opinion about this? Can we officially say that license texts are unnecessary in layout tests?

[Ojan Vafai]

On Mon, Jul 28, 2014 at 11:31 PM, Takeshi Yoshino <tyos...@chromium.org> wrote:

On Tue, Jul 29, 2014 at 3:04 PM, Ojan Vafai <oj...@chromium.org> wrote:

The following sections either don't have anything to do with style or don't match the common practice that I've seen, so I'd remove them:

Comparison (=== vs. ==)

Do you mean that this is not about style but just general JS common practice?

It is about style, but it's not common practice in layout tests. The vast majority use == and !=. The point of the style guide is to avoid people wasting time dealing with inconsistent style nits in code reviews. Has this really come up in code reviews?

  

License boilerplate

What's your opinion about this? Can we officially say that license texts are unnecessary in layout tests?

I don't really have an opinion about whether this should go in the style guide. In practice we don't put licenses in layout tests. I don't see this coming up in code reviews either though, so I don't know that it needs an explicit rule.

[Mounir Lamouri]

On Wed, 30 Jul 2014, at 04:17, Ojan Vafai wrote:
> > License boilerplate
> >>
> >
> > What's your opinion about this? Can we officially say that license texts
> > are unnecessary in layout tests?
> >
>
> I don't really have an opinion about whether this should go in the style
> guide. In practice we don't put licenses in layout tests. I don't see
> this
> coming up in code reviews either though, so I don't know that it needs an
> explicit rule.

Wouldn't that be better to make it explicit that the tests are in the
Public Domain? WHATWG recommend using Public Domain, CC0 1.0 Public
Domain to be precise [1] [2]. FWIW, Mozilla has been putting Public
Domain headers on its tests [3] [4] in order to ease contributions to
W3C/WHATWG test suites.

[1] http://wiki.whatwg.org/wiki/Testsuite#How_to_license_your_test_suite
[2] http://creativecommons.org/publicdomain/zero/1.0/
[3]
http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/reflect.js
[4]
http://mxr.mozilla.org/mozilla-central/search?string=public+domain&find=test&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central

-- Mounir

[Takeshi Yoshino]

On Wed, Jul 30, 2014 at 3:17 AM, Ojan Vafai <oj...@chromium.org> wrote:

On Mon, Jul 28, 2014 at 11:31 PM, Takeshi Yoshino <tyos...@chromium.org> wrote:

On Tue, Jul 29, 2014 at 3:04 PM, Ojan Vafai <oj...@chromium.org> wrote:

The following sections either don't have anything to do with style or don't match the common practice that I've seen, so I'd remove them:

Comparison (=== vs. ==)

Do you mean that this is not about style but just general JS common practice?

It is about style, but it's not common practice in layout tests. The vast majority use == and !=. The point of the style guide is to avoid people wasting time dealing with inconsistent style nits in code reviews. Has this really come up in code reviews?

I heard from one person that he's asked to use === in layout test files by some reviewer.

Codesearch under LayoutTests/ dir with extension of .js or .html returns

For / == / 5028

For / === / 1929

For / != /  3384

For / !== / 323

72% of equals are ==

91% of not equals are !=

OK, majority is double one, but count for === is not negligible. I'll rewrite it not to suggest triple ones strongly but say ok to use triple ones when it makes sense and use double ones if it's sufficient. Does it make sense?

 

  

License boilerplate

What's your opinion about this? Can we officially say that license texts are unnecessary in layout tests?

I don't really have an opinion about whether this should go in the style guide. In practice we don't put licenses in layout tests. I don't see this coming up in code reviews either though, so I don't know that it needs an explicit rule.

Yeah, we rarely waste time dealing with this point as all contributors should be guessing that it's not required because almost all files don't contain any boilerplate. (Code search for /\/\/ Copyright/ under LayoutTests directory excluding ietestcenter subdirs returns only 9 files).

I just thought it might be a little more healthy if we clearly say that rather than asking people to perceive from the repository as it's legal point.

[Adam Rice]

I would cast my vote in favour of js-test.js as I find the testharness.js way of doing asynchronous tests to be more verbose and harder to understand than the simpler method used by js-test.js. In particular, the test.step_func() function wrappers just look like noise to me.

While I initially felt that the *-expected.txt files were a gregarious example of over-specified testing, they uncovered quite a few bugs in the new WebSocket implementation, so I am grudgingly in favour of them.

Would it be possible to at least get a consensus against ad-hoc tests like http/tests/websocket/frame-lengths.html? It's cool that you can replicate the functionality of a test framework in three lines of Javascript, and I'm sure your way is better, now please stop. It's enough of a maintenance nightmare having two test frameworks without random tests doing their own thing.

Also, tests should not duplicate functionality from the test framework. The most common thing I see is

<div id="description"></div>

<div id="console"></div>

included in the HTML when js-test.js adds them automatically. It may necessary for layout-sensitive tests to specify where those should go, but I've never encountered a case where it was necessary in a pure JS test.

[ja...@hoppipolla.co.uk]

On Wednesday, 30 July 2014 10:46:42 UTC+1, Adam Rice wrote:

I would cast my vote in favour of js-test.js as I find the testharness.js way of doing asynchronous tests to be more verbose and harder to understand than the simpler method used by js-test.js. In particular, the test.step_func() function wrappers just look like noise to me.

For simple cases (i.e. one test in a file), these are no longer necessary; you can just write something like

<title>Test onload fires</title>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script>onload = done</script>

("simple" cases here actually tend to be appropriate for more complex tests where having more than one test per file doesn't really work anyway).

There is also an in-flight PR [1] to add native support for testing promise-based APIs, which will hopefully simplify testing of many near-future additions to the platform. Feedback on this (preferably on the critic review page [2] so we keep it all in one place) is very welcome.

[1] https://github.com/w3c/testharness.js/pull/82
[2] https://critic.hoppipolla.co.uk/r/2005

[Joshua Bell]

On Wed, Jul 30, 2014 at 2:46 AM, Adam Rice <ri...@chromium.org> wrote:

I would cast my vote in favour of js-test.js as I find the testharness.js way of doing asynchronous tests to be more verbose and harder to understand than the simpler method used by js-test.js. In particular, the test.step_func() function wrappers just look like noise to me.

While I initially felt that the *-expected.txt files were a gregarious example of over-specified testing, they uncovered quite a few bugs in the new WebSocket implementation, so I am grudgingly in favour of them.

Would it be possible to at least get a consensus against ad-hoc tests like http/tests/websocket/frame-lengths.html? It's cool that you can replicate the functionality of a test framework in three lines of Javascript, and I'm sure your way is better, now please stop. It's enough of a maintenance nightmare having two test frameworks without random tests doing their own thing.

Also, tests should not duplicate functionality from the test framework. The most common thing I see is

<div id="description"></div>

<div id="console"></div>

included in the HTML when js-test.js adds them automatically. It may necessary for layout-sensitive tests to specify where those should go, but I've never encountered a case where it was necessary in a pure JS test.

Having js-test.js add those automatically is relatively recent (within the past 24 months?). Likely you're seeing older tests and/or copy/paste of older tests. There's little incentive (and definitely some risk) in cleaning up older tests. Patches welcome maybe?

[philosophical rambling]

Tests using js-test tend to be more "literate"; through the copious use of eval()-based helpers, the output is usually readable and describes what the test is doing. As a side effect, it often captures subtle details that are not directly asserted - changes in exception messages, event ordering, and so forth.

Tests using testharness tend to be more minimal and fragile, stopping at the first failed assertion. On the flip side, this (seems) to lead to more rigor and careful structuring of the tests, ensuring that exactly one facet is being exercised per test. They do tend to be denser and harder to read, although the Promise-based additions (mentioned in a fork of this thread) will help a bunch.

Stepping back, I think we're discussing more than the difference between two testing frameworks. Are test scripts *content*, that "just" needs to render correctly to pass, or are they *code* that is actively probing the feature and will yield a pass/fail? While that's a philosophical distinction not firmly enforced by the frameworks, I do think the style of tests they encourage are biased towards the ends of that spectrum.

[/philosophical rambling]

On 30 July 2014 16:04, Takeshi Yoshino <tyos...@chromium.org> wrote:

On Wed, Jul 30, 2014 at 3:17 AM, Ojan Vafai <oj...@chromium.org> wrote:

On Mon, Jul 28, 2014 at 11:31 PM, Takeshi Yoshino <tyos...@chromium.org> wrote:

On Tue, Jul 29, 2014 at 3:04 PM, Ojan Vafai <oj...@chromium.org> wrote:

The following sections either don't have anything to do with style or don't match the common practice that I've seen, so I'd remove them:

Comparison (=== vs. ==)

Do you mean that this is not about style but just general JS common practice?

It is about style, but it's not common practice in layout tests. The vast majority use == and !=. The point of the style guide is to avoid people wasting time dealing with inconsistent style nits in code reviews. Has this really come up in code reviews?

I heard from one person that he's asked to use === in layout test files by some reviewer.

That was probably me. I'll try and stop.

 

Codesearch under LayoutTests/ dir with extension of .js or .html returns

For / == / 5028

For / === / 1929

For / != /  3384

For / !== / 323

72% of equals are ==

91% of not equals are !=

OK, majority is double one, but count for === is not negligible. I'll rewrite it not to suggest triple ones strongly but say ok to use triple ones when it makes sense and use double ones if it's sufficient. Does it make sense?

 

[rant]

Using == in JS either indicates you don't understand the funky type coercion that may be going on, or you do understand it and are confident that the values you are comparing aren't subject to coercion and want to save a character. Neither are convincing arguments to me for using it; it's sloppy, like relying on ASI. If tests are *code*, they shouldn't be sloppy. Ergo, always use ===. 

But consistency should win, so I'll shut up.

[/rant]

[Julien Chaffraix]

This document should cover internal tests as external tests should be
covered by the coding style of the organism you're uploading to (e.g
the W3C has its own style). As we are talking about Blink tests, we
should follow the blink style. As we change Blink coding style to
match Chromium's, we can have the tests follow but I wouldn't do the
other way around.

Julien

[Takeshi Yoshino]

Yeah, I just added this section to explain the scope of the style guide.

https://sites.google.com/a/chromium.org/dev/blink/coding-style/layout-test-style-guidelines#TOC-Scope-of-this-guideline

 

should follow the blink style. As we change Blink coding style to
match Chromium's, we can have the tests follow but I wouldn't do the
other way around.

OK. Any other opinion? If not I'll change the guide to encourage following

- http://www.chromium.org/blink/coding-style#TOC-Braces and

- http://www.chromium.org/blink/coding-style#TOC-Indentation

when writing new layout test files in Blink (not in sync with other test repository).

 

Julien

[Ojan Vafai]

I want to keep tests as minimal and simple as possible. Comments at the top of the file are not a noop. They actually affect what the test does. I'd be OK with someone putting a license at the top of the LayoutTests directory that applies to the whole subtree. I'm not qualified to say whether that sort of license is actually useful though.

In either case, this is a separate discussion. It's clearly not standard practice right now. I'm OK with the style guide explicitly saying that it's not required.

[Takeshi Yoshino]

On Thu, Jul 31, 2014 at 2:49 PM, Ojan Vafai <oj...@chromium.org> wrote:

On Tue, Jul 29, 2014 at 2:37 PM, Mounir Lamouri <mou...@lamouri.fr> wrote:

On Wed, 30 Jul 2014, at 04:17, Ojan Vafai wrote:
> >  License boilerplate
> >>
> >
> > What's your opinion about this? Can we officially say that license texts
> > are unnecessary in layout tests?
> >
>
> I don't really have an opinion about whether this should go in the style
> guide. In practice we don't put licenses in layout tests. I don't see
> this
> coming up in code reviews either though, so I don't know that it needs an
> explicit rule.

Wouldn't that be better to make it explicit that the tests are in the
Public Domain? WHATWG recommend using Public Domain, CC0 1.0 Public
Domain to be precise [1] [2]. FWIW, Mozilla has been putting Public
Domain headers on its tests [3] [4] in order to ease contributions to
W3C/WHATWG test suites.

[1] http://wiki.whatwg.org/wiki/Testsuite#How_to_license_your_test_suite
[2] http://creativecommons.org/publicdomain/zero/1.0/
[3]
http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/reflect.js
[4]
http://mxr.mozilla.org/mozilla-central/search?string=public+domain&find=test&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central

I want to keep tests as minimal and simple as possible. Comments at the top of the file are not a noop. They actually affect what the test does. I'd be OK with someone putting a license at the top of the LayoutTests directory that applies to the whole subtree.

Good suggestion :)

 

I'm not qualified to say whether that sort of license is actually useful though.

In either case, this is a separate discussion. It's clearly not standard practice right now. I'm OK with the style guide explicitly saying that it's not required.

Got it. It's been removed.

[Takeshi Yoshino]

On Thu, Jul 31, 2014 at 2:23 AM, Joshua Bell <jsb...@chromium.org> wrote:

On Wed, Jul 30, 2014 at 2:46 AM, Adam Rice <ri...@chromium.org> wrote:

On 30 July 2014 16:04, Takeshi Yoshino <tyos...@chromium.org> wrote:

On Wed, Jul 30, 2014 at 3:17 AM, Ojan Vafai <oj...@chromium.org> wrote:

On Mon, Jul 28, 2014 at 11:31 PM, Takeshi Yoshino <tyos...@chromium.org> wrote:

On Tue, Jul 29, 2014 at 3:04 PM, Ojan Vafai <oj...@chromium.org> wrote:

The following sections either don't have anything to do with style or don't match the common practice that I've seen, so I'd remove them:

Comparison (=== vs. ==)

Do you mean that this is not about style but just general JS common practice?

It is about style, but it's not common practice in layout tests. The vast majority use == and !=. The point of the style guide is to avoid people wasting time dealing with inconsistent style nits in code reviews. Has this really come up in code reviews?

I heard from one person that he's asked to use === in layout test files by some reviewer.

That was probably me. I'll try and stop.

 

Codesearch under LayoutTests/ dir with extension of .js or .html returns

For / == / 5028

For / === / 1929

For / != /  3384

For / !== / 323

72% of equals are ==

91% of not equals are !=

OK, majority is double one, but count for === is not negligible. I'll rewrite it not to suggest triple ones strongly but say ok to use triple ones when it makes sense and use double ones if it's sufficient. Does it make sense?

 

[rant]

Using == in JS either indicates you don't understand the funky type coercion that may be going on, or you do understand it and are confident that the values you are comparing aren't subject to coercion and want to save a character. Neither are convincing arguments to me for using it; it's sloppy, like relying on ASI. If tests are *code*, they shouldn't be sloppy. Ergo, always use ===. 

But consistency should win, so I'll shut up.

[/rant]

Thanks. I understand both that === is preferred for testing and importance of consistency.

I hope the new text is explaining both points fairly.

https://sites.google.com/a/chromium.org/dev/blink/coding-style/layout-test-style-guidelines?pli=1#TOC-Comparison-vs.-

 

[Takeshi Yoshino]

As Blink-in-JS started, there will be more JS files also in the Source/ directory.

For now, there're 344 JS files in the devtools directory and 15 files in the others. Most of the files are following the Blink C++ style in both.

I also heard from some people that they're wondering if it's better or not using Chromium style for new Blink-in-JS code considering upcoming merge of repositories.