Bug 38987 - [Qt] The QScriptValue autotest should be refactored.
Summary: [Qt] The QScriptValue autotest should be refactored.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jędrzej Nowacki
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 36600
  Show dependency treegraph
 
Reported: 2010-05-12 06:20 PDT by Jędrzej Nowacki
Modified: 2010-05-21 06:42 PDT (History)
5 users (show)

See Also:


Attachments
Fix v1 (185.37 KB, patch)
2010-05-12 06:40 PDT, Jędrzej Nowacki
no flags Details | Formatted Diff | Diff
Fix v1.01 (185.33 KB, patch)
2010-05-12 08:29 PDT, Jędrzej Nowacki
no flags Details | Formatted Diff | Diff
Fix v2 (185.34 KB, patch)
2010-05-19 09:30 PDT, Jędrzej Nowacki
kenneth: review-
kenneth: commit-queue-
Details | Formatted Diff | Diff
Fix v3 (185.34 KB, patch)
2010-05-20 00:19 PDT, Jędrzej Nowacki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jędrzej Nowacki 2010-05-12 06:20:49 PDT
QScriptValue autotest have great coverage and it is really useful, but it starts to be too big. The main part of it is a generated source code (100KB). The part should be divided into several pieces to increase maintainability and compiling time (right now it doesn't profit from distributed compiling and it takes ages to compile it with MSVS2008).

Summary:
 - tst_qscriptvalue_generated.cpp should be divided into a several files.
 - tests should be modified to reduce amount of optimizations taken by different compilers.
Comment 1 Jędrzej Nowacki 2010-05-12 06:40:34 PDT
Created attachment 55839 [details]
Fix v1

Patch overview:
1. tst_qscriptvalue_generated.cpp was removed.
2. 3 new files were created with content of the removed file.
 - tst_qscriptvalue_generated_init.cpp - contains test data set (tested values).
 - tst_qscriptvalue_generated_isXXX.cpp - contains expected results and tests for QScriptValue::isXXX methods.
 - tst_qscriptvalue_generated_toXXX.cpp -  contains expected results and tests for QScriptValue::toXXX methods.
3. typical test was changed; expected values are loaded via loops and static arrays (instead of nearby infinite numbers of insert() and operator<<() calls :-) ).
4. expected values weren't changed.
5. the same content (methods, values) is tested.
6. all tests pass.
Comment 2 WebKit Review Bot 2010-05-12 06:44:20 PDT
Attachment 55839 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
Last 3072 characters of output:
[4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:34:  Extra space before [  [whitespace/braces] [5]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:166:  Missing space after ,  [whitespace/comma] [3]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:168:  Extra space before [  [whitespace/braces] [5]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:263:  Extra space before [  [whitespace/braces] [5]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:395:  Missing space after ,  [whitespace/comma] [3]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:396:  Extra space before [  [whitespace/braces] [5]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:447:  Extra space before [  [whitespace/braces] [5]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:579:  Missing space after ,  [whitespace/comma] [3]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:581:  Extra space before [  [whitespace/braces] [5]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:676:  Extra space before [  [whitespace/braces] [5]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:808:  Missing space after ,  [whitespace/comma] [3]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:810:  Extra space before [  [whitespace/braces] [5]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:905:  Extra space before [  [whitespace/braces] [5]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:1037:  Missing space after ,  [whitespace/comma] [3]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:1038:  Extra space before [  [whitespace/braces] [5]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:1085:  Extra space before [  [whitespace/braces] [5]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:1217:  Missing space after ,  [whitespace/comma] [3]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:1219:  Extra space before [  [whitespace/braces] [5]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:1314:  Extra space before [  [whitespace/braces] [5]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:1446:  Missing space after ,  [whitespace/comma] [3]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:1448:  Extra space before [  [whitespace/braces] [5]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:1543:  Extra space before [  [whitespace/braces] [5]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:1675:  Missing space after ,  [whitespace/comma] [3]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:1677:  Extra space before [  [whitespace/braces] [5]
Total errors found: 45 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Jędrzej Nowacki 2010-05-12 08:29:49 PDT
Created attachment 55849 [details]
Fix v1.01
Comment 4 WebKit Review Bot 2010-05-12 08:32:27 PDT
Attachment 55849 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_isXXX.cpp:25:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_init.cpp:24:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_toXXX.cpp:24:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Jędrzej Nowacki 2010-05-12 08:35:32 PDT
(In reply to comment #4)
> Attachment 55849 [details] did not pass style-queue:
> (...)
> If any of these errors are false positives, please file a bug against check-webkit-style.
check-webkit-style bug 35151
Comment 6 Kenneth Rohde Christiansen 2010-05-18 10:42:32 PDT
Comment on attachment 55849 [details]
Fix v1.01

\I didn't really like the "isXXX, toXXX". What about toType, isType, or maybe tst_qscriptvalue_generated_typeValidation, _typeConversion?
Comment 7 Jędrzej Nowacki 2010-05-19 09:30:43 PDT
Created attachment 56494 [details]
Fix v2

(In reply to comment #6)
> (From update of attachment 55849 [details])
> \I didn't really like the "isXXX, toXXX". What about toType, isType, or maybe tst_qscriptvalue_generated_typeValidation, _typeConversion?
Names were changed to totype and istype.
Comment 8 WebKit Review Bot 2010-05-19 09:35:06 PDT
Attachment 56494 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_istype.cpp:25:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_totype.cpp:24:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_init.cpp:24:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Kenneth Rohde Christiansen 2010-05-19 10:28:01 PDT
Comment on attachment 56494 [details]
Fix v2

JavaScriptCore/ChangeLog:25
 +          * qt/tests/qscriptvalue/tst_qscriptvalue_generated_isXXX.cpp: Added.
You didn't update the ChangeLog, r-
Comment 10 Jędrzej Nowacki 2010-05-20 00:19:51 PDT
Created attachment 56568 [details]
Fix v3

(In reply to comment #9)
> (From update of attachment 56494 [details])
> JavaScriptCore/ChangeLog:25
>  +          * qt/tests/qscriptvalue/tst_qscriptvalue_generated_isXXX.cpp: Added.
> You didn't update the ChangeLog, r-
Oops :-)
Comment 11 WebKit Review Bot 2010-05-20 00:25:59 PDT
Attachment 56568 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_istype.cpp:25:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_totype.cpp:24:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_init.cpp:24:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 WebKit Commit Bot 2010-05-21 06:42:42 PDT
Comment on attachment 56568 [details]
Fix v3

Clearing flags on attachment: 56568

Committed r59930: <http://trac.webkit.org/changeset/59930>
Comment 13 WebKit Commit Bot 2010-05-21 06:42:50 PDT
All reviewed patches have been landed.  Closing bug.