Bug 201778 – [results.webkit.org] Test library for js library

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Zhifei Fang]

We will need to add some tests for the JS library I wrote. It is getting more and more complex.

In this bug, I will introduce a test framework with some fundamental test cases for Ref.js

In the meanwhile, the Test.js can be reused for any other application that use Ref.js. 

Also, a test app is included, people can directly run the tests by open the test/index.html

[Zhifei Fang]

Created attachment 378756 [details]
Patch

[Jonathan Bedard]

Comment on attachment 378756 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378756&action=review

> Tools/ChangeLog:3
> +        Add a test library and some fundamental test for Ref.js.

Would just say 'Add tests for Ref.js'

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/Ref.js:220
> +    if (diff === undefined)

This seems like a stand-alone change. Can we bring it out into a different bug and just relate to this one?

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/Test.js:3
> +class AssertFailedError extends Error {

Any reason we need a specific error class? In my experience, such generic base classes are rarely helpful.

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/Test.js:13
> +class Expect {

I haven't seen this style of assert before. Is it based off of something you've seen and is there a reason you picked it? I'm more familiar with the assert<condition>(valueA, valueB) model, like Python uses. Not necessarily opposed to this, but I feel like it does require justification.

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/Test.js:131
> +    for (let method of testObjMethods) {

We aren't actually checking if the methods are 'test' methods, but you've named all of you tests that way.

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/Test.js:228
> +            haveErrsor = await this.runTest(testClassName, testFnName);

Typo

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/components/TestComponents.js:31
> +    return (

Why the parenthesis?

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/test/index.html:18
> +                // Test file list: Root Path will be Test.js's folder

Comment is not needed.

[Zhifei Fang]

(In reply to Jonathan Bedard from comment #2)
> Comment on attachment 378756 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378756&action=review
> 
> > Tools/ChangeLog:3
> > +        Add a test library and some fundamental test for Ref.js.
> 
> Would just say 'Add tests for Ref.js'
> 
> > Tools/resultsdbpy/resultsdbpy/view/static/library/js/Ref.js:220
> > +    if (diff === undefined)
> 
> This seems like a stand-alone change. Can we bring it out into a different
> bug and just relate to this one?

This will fix a test failure.

> 
> > Tools/resultsdbpy/resultsdbpy/view/static/library/js/Test.js:3
> > +class AssertFailedError extends Error {
> 
> Any reason we need a specific error class? In my experience, such generic
> base classes are rarely helpful.

This will help us to identify if the test is really failed or there are some other issue in the test code, it could throw an error which is not "AssertFailedError"
> 
> > Tools/resultsdbpy/resultsdbpy/view/static/library/js/Test.js:13
> > +class Expect {
> 
> I haven't seen this style of assert before. Is it based off of something
> you've seen and is there a reason you picked it? I'm more familiar with the
> assert<condition>(valueA, valueB) model, like Python uses. Not necessarily
> opposed to this, but I feel like it does require justification.
> 
See it here,
https://github.com/Automattic/expect.js

This is BDD assertion style. 

> > Tools/resultsdbpy/resultsdbpy/view/static/library/js/Test.js:131
> > +    for (let method of testObjMethods) {
> 
> We aren't actually checking if the methods are 'test' methods, but you've
> named all of you tests that way.
Line 128 will gathering all base class method name, line 132 will filter all those functions, all other user defined method will be treated as test method
> 
> > Tools/resultsdbpy/resultsdbpy/view/static/library/js/Test.js:228
> > +            haveErrsor = await this.runTest(testClassName, testFnName);
> 
> Typo
> 
> > Tools/resultsdbpy/resultsdbpy/view/static/library/js/components/TestComponents.js:31
> > +    return (
> 
> Why the parenthesis?

Js won't allow you return \n something
> 
> > Tools/resultsdbpy/resultsdbpy/view/static/library/js/test/index.html:18
> > +                // Test file list: Root Path will be Test.js's folder
> 
> Comment is not needed.

[Zhifei Fang]

> 
> > Tools/resultsdbpy/resultsdbpy/view/static/library/js/test/index.html:18
> > +                // Test file list: Root Path will be Test.js's folder
> 
> Comment is not needed.

I think this is still needed, please be advise that index.html and Test.js in different directory, as we dynamically import a file, the relative path will be resolved using Test.js's root folder.

[Zhifei Fang]

Created attachment 379053 [details]
Patch

[Jonathan Bedard]

Comment on attachment 379053 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379053&action=review

> Tools/ChangeLog:9
> +        (applyStateDiff): For singal value, null, 0, false those are still valid state

Change to: 'null, 0 and false are all valid state values.'

> Tools/ChangeLog:13
> +        (Expect): Expect class help user perform a expectation assertion

Change to: Perform assertions during unit tests.

> Tools/ChangeLog:22
> +        (Test): Common Test class for user to extend. 

Base class for test suite. (Also, should this be named suite? A test is a specific function, this class is a collection of tests, no?)

> Tools/ChangeLog:23
> +        (Test.prototype.expect): This expect method will help the test perform expectation assertions

I think this comment might make things more confusing.

> Tools/ChangeLog:24
> +        (Test.prototype.sleep): Test will sleep for certain ms

Don't abbreviate milliseconds here

> Tools/ChangeLog:26
> +        (Test.prototype.waitForRefMounted): Wait until we revice ref's onElementMount signal with timeout

By 'ref', do you mean the Ref object? Probably need to clarify.

> Tools/ChangeLog:29
> +        (Test.prototype.async.setup): Common interface for setup a test class

Might turn into 'Setup test suite' if the classname changes.

> Tools/ChangeLog:30
> +        (Test.prototype.async.clearUp): Common interface for clearup a test class

'cleanUp' would be the traditional name for this.

> Tools/ChangeLog:31
> +        (getTestFucntionNames): Collect all the test method of a test instance.

'...all test methods of a...'

> Tools/ChangeLog:34
> +        (async.getTestResult): This will run the test and generate a TestResult object

Run the test and generate a TestResult object.

> Tools/ChangeLog:35
> +        (TestController): Control how the test running

I think this comment makes things more confusing.

> Tools/ChangeLog:36
> +        (TestController.prototype.addResultHandler): Result handler should be hook with a component or something else to render the test result

Could make this shorter...a result handler just needs to return renderable html, right? Not sure you need to call out components specifically

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/Test.js:88
> +    sleep(ms) {

Don't abbreviate milliseconds here

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/Test.js:121
> +    /*Common interface*/

Not needed

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/Test.js:190
> +        if ( testName in this.allTests) {

Extra space after (

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/components/TestComponents.js:3
> +

Let's be consistent, you're using one space everywhere else

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/components/TestComponents.js:12
> +            ${TestRenderArea(testController)}

Let's be consistent on the tabs.

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/components/TestComponents.js:32
> +        `

Why the newline?

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/test/index.html:5
> +        <link rel="stylesheet" href="https://results.safari.apple.com/library/css/webkit.css"></link>

Shouldn't this be a relative local path?

[Zhifei Fang]

Comment on attachment 379053 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379053&action=review

>> Tools/ChangeLog:22
>> +        (Test): Common Test class for user to extend. 
> 
> Base class for test suite. (Also, should this be named suite? A test is a specific function, this class is a collection of tests, no?)

Good idea

>> Tools/ChangeLog:26
>> +        (Test.prototype.waitForRefMounted): Wait until we revice ref's onElementMount signal with timeout
> 
> By 'ref', do you mean the Ref object? Probably need to clarify.

Yes

>> Tools/ChangeLog:36
>> +        (TestController.prototype.addResultHandler): Result handler should be hook with a component or something else to render the test result
> 
> Could make this shorter...a result handler just needs to return renderable html, right? Not sure you need to call out components specifically

This is where the view will receive the test result notification, it can be a html based, or CLI based if running in node.js

[Zhifei Fang]

Created attachment 379265 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 379265 [details]
Patch

Clearing flags on attachment: 379265

Committed r250307: <https://trac.webkit.org/changeset/250307>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55668433>