(In reply to comment #2)
> Maybe Saam can take this one down.
Sure, but we already do something similar:
`
var x = {};
x.foo()
`
==>
"Exception: TypeError: undefined is not a function (evaluating 'x.foo()')"
Maybe we should just switch up the wording? I personally like it the way it is now,
because it tells you both what the problem is and where the problem is
(Chrome just says where it is, but not that it's undefined).
Maybe we should make the x.foo() part more prominent and not in parens, something like:
"Exception: TypeError: when evaluating 'x.foo()': undefined is not a function"
I think our current wording is more precise but less communicative.
"undefined is not a function" is a precise definition of the error processed by the VM.
"x.y is not a function" -- or, perhaps better, "x.y is undefined" -- is a communicative description of what the developer should go debug and/or fix.
I prefer the communicative approach even though it is less precise. No one has ever said, "I love C++ template error messages -- they are so precise!" An extreme example, but still relevant, I think.
I think the best option is text context followed by communicative error:
x.y(): x.y is undefined (TypeError)
Created attachment 247212[details]
work in progress
What do you guys think of the format of this change?
It would allow for easy modification in the future of
error messages because we can just pass in a function
to do the rewriting.
The strings it produces now are:
"TypeError: evaluating 'x.bar()', undefined is not a function"
"TypeError: evaluating '(10)()', 10 is not a function"
We can change this specific text and play around with it,
but more work needs to be done if we want to just extract "x.bar"
and also change the words "undefined/10/etc is not a function"
because the error object itself is created with this string.
We may need to tack on more meta data to the error message to
capture the "undefined/10/etc".
Looking through the code that throws exceptions in the VM in
VM.cpp::throwException, what code paths will trigger
if (callFrame && callFrame->codeBlock())
to not be true?
Also, what code paths cause the code block to not have expression info?
i.e, when will "(!callFrame->codeBlock()->hasExpressionInfo())" be true?
Attachment 247212[details] did not pass style-queue:
ERROR: Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:131: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:141: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
Total errors found: 2 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 247215[details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 247217[details]
Archive of layout-test-results from ews100 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
> Looking through the code that throws exceptions in the VM in
> VM.cpp::throwException, what code paths will trigger
> if (callFrame && callFrame->codeBlock())
> to not be true?
codeBlock() is null if the callee is a host function or host object.
(In reply to comment #12)
> Darn, seems like we do indeed need more metadata.
Yeah it depends on if we want to split out the "undefined"
from "undefined is not a function". Or we can hack it and
do some string processing when generating the error message.
What do you think about the approach of keeping a function pointer
member variable as a way to transform the error message? Obviously this
is a bit slower than before, but I'm not sure if it will be
a noticeable slow down (still need to run perf tests). It will also allow for
future flexibility. Not sure if it's worth it or not.
Also, ErrorInstance would be 7 bytes larger.
> What do you think about the approach of keeping a function pointer
> member variable as a way to transform the error message? Obviously this
> is a bit slower than before, but I'm not sure if it will be
> a noticeable slow down (still need to run perf tests). It will also allow for
> future flexibility. Not sure if it's worth it or not.
> Also, ErrorInstance would be 7 bytes larger.
Size of error object doesn't matter, and string processing during throw is OK.
Created attachment 247649[details]
work in progress
I may want to abstract a couple more things into RuntimeType.
Still need to update the tests that deal w/ error messages.
This patch creates error messages like:
>>> var x = 20
undefined
>>> x()
Exception: TypeError: x is not a function, it is an Integer (20).
>>> var x = []
undefined
>>> x()
Exception: TypeError: x is not a function, it is an Object (Array).
>>> function Ctor(){}
undefined
>>> x = new Ctor
[object Object]
>>> x()
Exception: TypeError: x is not a function, it is an Object (Ctor).
>>> x = 20.2
20.2
>>> x()
Exception: TypeError: x is not a function, it is a Number (20.2).
>>> x = null
null
>>> x()
Exception: TypeError: x is not a function, it is null.
>>> x = undefined
undefined
>>> x()
Exception: TypeError: x is not a function, it is undefined.
Attachment 247706[details] did not pass style-queue:
ERROR: Source/JavaScriptCore/runtime/ErrorInstance.h:26: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:134: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
Total errors found: 2 in 13 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 247706[details]
work in progress
View in context: https://bugs.webkit.org/attachment.cgi?id=247706&action=review> Source/JavaScriptCore/runtime/ErrorInstance.h:54
> + typedef String (*MessageGenerator) (const String& originalMessage, const String& sourceText, RuntimeType, SourceTextWhereErrorOccurred);
I need a better name for this.
> Source/JavaScriptCore/runtime/ErrorInstance.h:70
> + RuntimeType m_runtimeType;
And a better name for this.
Any ideas?
Created attachment 247729[details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 247730[details]
Archive of layout-test-results from ews100 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
(In reply to comment #23)
> Created attachment 248158[details]
> work in progress
>
> Closer. Need to change a few tests inside run-javascriptcore-tests to
> reflect new error messages.
Nevermind, looks like run-javascriptcore-tests pulls in the latest inside LayoutTests/js
Comment on attachment 248404[details]
patch
View in context: https://bugs.webkit.org/attachment.cgi?id=248404&action=review
Some drive by comments.
> Source/JavaScriptCore/interpreter/Interpreter.cpp:145
> + callFrame->vm().throwException(callFrame, createInvalidParameterError(callFrame, "Function.prototype.apply", "an Array-like object", arguments));
Nit: Should have a test covering this as well.
> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:109
> - return jsCast<JSString*>(v.asCell());
> + return jsString(&vm, makeString('"', asString(v)->value(exec), '"'));
Should this escape double quotes in the string? Maybe truncate too? We should make sure there is a test covering this, since this looks like a change uncovered by the tests that are updated.
Nit: Also, since we know the string will have at least 2 characters you can use jsNontrivialString.
> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:190
> + builder.append(" is not a function, it is ");
Nit: Any of the StringBuilder appends with a non-single-character can be appendLiteral.
> Source/JavaScriptCore/runtime/RuntimeType.cpp:16
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of Apple Inc. ("Apple") nor the names of
> + * its contributors may be used to endorse or promote products derived
> + * from this software without specific prior written permission.
Nit: We normally just have a 2 clause license nowadays. Unless you are using the 3 clause for a specific reason, new files typically have the 2 clause. See other copyright 2015 files. (Same comment applies to RuntimeType.h)
> Source/JavaScriptCore/runtime/RuntimeType.cpp:41
> +RuntimeType getRuntimeTypeForValue(JSValue v)
> +{
> + RuntimeType ret;
Style: WebKit style is to not abbreviate variables names. These can be "value" and "returnType". Or you could early return in the sections below instead of the if/else chain and avoid the "ret" local.
> Source/JavaScriptCore/runtime/RuntimeType.cpp:86
> + return "";
Nit: emptyString()
> Source/JavaScriptCore/runtime/RuntimeType.cpp:89
> +
> +
Nit: extra newline.
> Source/JavaScriptCore/runtime/RuntimeType.h:37
> +enum RuntimeType : uint8_t {
Is it worth adding Symbol type now? Would this hit the RELEASE_ASSERT_NOT_REACHED's in runtimeTypeAsString?
Comment on attachment 248404[details]
patch
View in context: https://bugs.webkit.org/attachment.cgi?id=248404&action=review
Need a new rebased version of this, since the patch does not apply.
> Source/JavaScriptCore/runtime/ErrorInstance.cpp:36
> + , m_sourceAppender(nullptr)
> + , m_runtimeTypeForCause(TypeNothing)
Won’t need these if we initialize them in the header.
> Source/JavaScriptCore/runtime/ErrorInstance.h:70
> + SourceAppender m_sourceAppender;
> + RuntimeType m_runtimeTypeForCause;
Should initialize these here:
SourceAppender m_sourceAppender { nullptr };
RuntimeType m_runtimeTypeForCause { TypeNothing };
> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:184
> + size_t notAFunctionIdx = originalMessage.reverseFind(ASCIILiteral("is not a function"));
It’s strange that reverseFind is not overloaded so you can use a literal with it. It’s safer to use auto for the return type rather than size_t. In WebKit coding style we do not use abbreviations like "idx".
> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:186
> + String displayValue = originalMessage.left(notAFunctionIdx - 1);
Could use a StringView for this to avoid creating an unnecessary temporary string.
> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:190
> + builder.append(" is not a function, it is ");
Should use appendLiteral.
> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:193
> + builder.append("an ");
Should use appendLiteral.
> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:195
> + builder.append("a ");
Should use appendLiteral.
> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:197
> + builder.append(" (");
Should use appendLiteral.
> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:199
> + builder.append(")");
Should use appendLiteral.
(In reply to comment #27)
> Comment on attachment 248404[details]
> patch
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248404&action=review
>
> Some drive by comments.
>
> > Source/JavaScriptCore/interpreter/Interpreter.cpp:145
> > + callFrame->vm().throwException(callFrame, createInvalidParameterError(callFrame, "Function.prototype.apply", "an Array-like object", arguments));
>
> Nit: Should have a test covering this as well.
>
> > Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:109
> > - return jsCast<JSString*>(v.asCell());
> > + return jsString(&vm, makeString('"', asString(v)->value(exec), '"'));
>
> Should this escape double quotes in the string? Maybe truncate too? We
> should make sure there is a test covering this, since this looks like a
> change uncovered by the tests that are updated.
Good point, I didn't think about this. I'm not certain what we should do
here. I'll think on it.
>
> Nit: Also, since we know the string will have at least 2 characters you can
> use jsNontrivialString.
>
> > Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:190
> > + builder.append(" is not a function, it is ");
>
> Nit: Any of the StringBuilder appends with a non-single-character can be
> appendLiteral.
>
> > Source/JavaScriptCore/runtime/RuntimeType.cpp:16
> > + * 1. Redistributions of source code must retain the above copyright
> > + * notice, this list of conditions and the following disclaimer.
> > + * 2. Redistributions in binary form must reproduce the above copyright
> > + * notice, this list of conditions and the following disclaimer in the
> > + * documentation and/or other materials provided with the distribution.
> > + * 3. Neither the name of Apple Inc. ("Apple") nor the names of
> > + * its contributors may be used to endorse or promote products derived
> > + * from this software without specific prior written permission.
>
> Nit: We normally just have a 2 clause license nowadays. Unless you are using
> the 3 clause for a specific reason, new files typically have the 2 clause.
> See other copyright 2015 files. (Same comment applies to RuntimeType.h)
>
> > Source/JavaScriptCore/runtime/RuntimeType.cpp:41
> > +RuntimeType getRuntimeTypeForValue(JSValue v)
> > +{
> > + RuntimeType ret;
>
> Style: WebKit style is to not abbreviate variables names. These can be
> "value" and "returnType". Or you could early return in the sections below
> instead of the if/else chain and avoid the "ret" local.
>
> > Source/JavaScriptCore/runtime/RuntimeType.cpp:86
> > + return "";
>
> Nit: emptyString()
>
> > Source/JavaScriptCore/runtime/RuntimeType.cpp:89
> > +
> > +
>
> Nit: extra newline.
>
> > Source/JavaScriptCore/runtime/RuntimeType.h:37
> > +enum RuntimeType : uint8_t {
>
> Is it worth adding Symbol type now? Would this hit the
> RELEASE_ASSERT_NOT_REACHED's in runtimeTypeAsString?
I think this is worthy of its own patch. It's the next one on my list
and shouldn't be too difficult to implement.
Thanks for the comments. I'll address them. Also, I need
to go through and comment on a few things myself to get feedback.
Comment on attachment 248404[details]
patch
View in context: https://bugs.webkit.org/attachment.cgi?id=248404&action=review> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:141
> + if (sourceText[idx] != ')') {
I want to make a test for this.
> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:156
> + if (isInMultiLineComment) {
I also want to make a test for this.
(In reply to comment #28)
> Comment on attachment 248404[details]
> patch
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248404&action=review>
> It’s strange that reverseFind is not overloaded so you can use a literal
> with it. It’s safer to use auto for the return type rather than size_t. In
> WebKit coding style we do not use abbreviations like "idx".
>
I think I should drop the ASCIILiteral here,
it seems like the type conversion to reverseFind goes:
ASCIILiteral => const char* => WTFString
which doesn't make much sense.
Comment on attachment 248432[details]
patch
View in context: https://bugs.webkit.org/attachment.cgi?id=248432&action=review> LayoutTests/js/exception-for-nonobject-expected.txt:7
> +PASS 1 instanceof {}.undefined threw exception TypeError: undefined is not a Function. 'instanceof' expects a Function as its argument. (evaluating '1 instanceof {}.undefined').
Is it possible to make this say "{}.undefined is not a function"?
This message is probably better off without "'instanceof' expects a Function as its argument. That seems redundant.
> LayoutTests/js/instance-of-immediates-expected.txt:6
> +PASS (1 instanceof 1) threw exception TypeError: 1 is not a Function. 'instanceof' expects a Function as its argument. (evaluating '1 instanceof 1').
Ditto.
> LayoutTests/js/typedarray-constructors-expected.txt:6
> +PASS Int8Array() threw exception TypeError: Int8Array is not a function, it is an Object (Function).
Is it possible to make this say "TypeError: Int8Array (Object) is not a function (evaluating 'Int8Array()')"?
Created attachment 248437[details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
(In reply to comment #33)
> Comment on attachment 248432[details]
> patch
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248432&action=review
>
> > LayoutTests/js/exception-for-nonobject-expected.txt:7
> > +PASS 1 instanceof {}.undefined threw exception TypeError: undefined is not a Function. 'instanceof' expects a Function as its argument. (evaluating '1 instanceof {}.undefined').
>
> Is it possible to make this say "{}.undefined is not a function"?
Should be doable.
>
> This message is probably better off without "'instanceof' expects a Function
> as its argument. That seems redundant.
>
> > LayoutTests/js/instance-of-immediates-expected.txt:6
> > +PASS (1 instanceof 1) threw exception TypeError: 1 is not a Function. 'instanceof' expects a Function as its argument. (evaluating '1 instanceof 1').
>
> Ditto.
>
> > LayoutTests/js/typedarray-constructors-expected.txt:6
> > +PASS Int8Array() threw exception TypeError: Int8Array is not a function, it is an Object (Function).
>
> Is it possible to make this say "TypeError: Int8Array (Object) is not a
> function (evaluating 'Int8Array()')"?
Should we just specify that it's simply an Object or should we specify the name of constructor like:
TypeError: <call> (Constructor) is no a function (evaluation '<expr>')?
For example
function Foo() {}
var x = new Foo
x()
should this throw
TypeError: x (Foo) is not a function (evaluating 'x()')
or
TypeError: x (Object) is not a function (evaluating 'x()')
Comment on attachment 248432[details]
patch
View in context: https://bugs.webkit.org/attachment.cgi?id=248432&action=review> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:223
> + return createError(exec, createTypeError, value, makeString("is not ", expectedType, ". '", op, "' expects ", expectedType, " as its argument."), defaultSourceAppender);
Looks like I also need to write a test for invalid parameter for "in" expressions
(In reply to comment #33)
> Comment on attachment 248432[details]
> patch
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248432&action=review
>
> > LayoutTests/js/exception-for-nonobject-expected.txt:7
> > +PASS 1 instanceof {}.undefined threw exception TypeError: undefined is not a Function. 'instanceof' expects a Function as its argument. (evaluating '1 instanceof {}.undefined').
>
> Is it possible to make this say "{}.undefined is not a function"?
Geoff, there is one stupid edge case that will make string processing here
not work. For example, all variations of instanceof occurring on left or right
of operator. i.e
"instanceof" instanceof "instanceof"
"x" instanceof "instanceof"
"instanceof" instanceof "x"
Maybe it's just worth ignoring this?
Or we can only do it when we're sure that there is only one instanceof in the
source text (which will probably be as close to 100% of the time as we can get),
like checking that find("instanceof") == reverseFind("instanceof").
Created attachment 248440[details]
Archive of layout-test-results from ews101 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
> Should we just specify that it's simply an Object or should we specify the
> name of constructor like:
> TypeError: <call> (Constructor) is no a function (evaluation '<expr>')?
> For example
>
> function Foo() {}
> var x = new Foo
> x()
>
> should this throw
> TypeError: x (Foo) is not a function (evaluating 'x()')
> or
> TypeError: x (Object) is not a function (evaluating 'x()')
Hmmm...
I suppose the ideal solution is some kind of debug-only serialization for values. For example, a number might want to tell you that it is a number, and its value. Same for a boolean. An object might want to tell you its constructor's name, or perhaps some of its properties or something.
In this case, I don't love "Foo" as a stand-alone identifier because it's ambiguous between "the function named Foo" and "an object allocated by the function named Foo".
I guess maybe for now what we want is:
TypeError: x is not a function.
And long-term:
TypeError: x is not a function. (In 'x()', 'x' is { }).
And for other x's:
TypeError: x is not a function. (In 'x()', 'x' is { prop1: 12, prop2:... }).
TypeError: x is not a function. (In 'x()', 'x' is 7).
TypeError: x is not a function. (In 'x()', 'x' is false).
TypeError: x is not a function. (In 'x()', 'x' is function Foo(z) { var x = ...}).
(In reply to comment #41)
> > Should we just specify that it's simply an Object or should we specify the
> > name of constructor like:
> > TypeError: <call> (Constructor) is no a function (evaluation '<expr>')?
> > For example
> >
> > function Foo() {}
> > var x = new Foo
> > x()
> >
> > should this throw
> > TypeError: x (Foo) is not a function (evaluating 'x()')
> > or
> > TypeError: x (Object) is not a function (evaluating 'x()')
>
> Hmmm...
>
> I suppose the ideal solution is some kind of debug-only serialization for
> values. For example, a number might want to tell you that it is a number,
> and its value. Same for a boolean. An object might want to tell you its
> constructor's name, or perhaps some of its properties or something.
>
> In this case, I don't love "Foo" as a stand-alone identifier because it's
> ambiguous between "the function named Foo" and "an object allocated by the
> function named Foo".
>
> I guess maybe for now what we want is:
>
> TypeError: x is not a function.
>
> And long-term:
>
> TypeError: x is not a function. (In 'x()', 'x' is { }).
>
> And for other x's:
> TypeError: x is not a function. (In 'x()', 'x' is { prop1: 12, prop2:... }).
> TypeError: x is not a function. (In 'x()', 'x' is 7).
> TypeError: x is not a function. (In 'x()', 'x' is false).
> TypeError: x is not a function. (In 'x()', 'x' is function Foo(z) { var x =
> ...}).
I think we should just do this now, and we can make it more comprehensive later.
But we have almost everything we need except greater fidelity for objects.
I think for objects we should just do:
TypeError: x is not a function. (In 'x()', 'x' is an instance of ConstructorName).
Created attachment 248479[details]
patch
(Uploading patch w/out tests so I can see which tests I need to run for these
error messages b/c running all of run-webkit-tests are flaky on my machine).
Created attachment 248486[details]
Archive of layout-test-results from ews103 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 248487[details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
(In reply to comment #41)
> TypeError: x is not a function.
>
> And long-term:
>
> TypeError: x is not a function. (In 'x()', 'x' is { }).
>
> And for other x's:
> TypeError: x is not a function. (In 'x()', 'x' is { prop1: 12, prop2:... }).
> TypeError: x is not a function. (In 'x()', 'x' is 7).
> TypeError: x is not a function. (In 'x()', 'x' is false).
> TypeError: x is not a function. (In 'x()', 'x' is function Foo(z) { var x =
> ...}).
We need to make sure that when producing these error messages we do not call into JS -- no implicit conversions, no getters, etc
Created attachment 248551[details]
Archive of layout-test-results from ews102 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 248548[details]
patch
View in context: https://bugs.webkit.org/attachment.cgi?id=248548&action=review
r=me if you fix the function name and the failing tests
> Source/JavaScriptCore/runtime/RuntimeType.h:47
> +RuntimeType getRuntimeTypeForValue(JSValue);
We reserve the word "get" for functions returning out parameters. This should just be runtimeTypeForValue.
(In reply to comment #54)
> Comment on attachment 248548[details]
> patch
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248548&action=review
>
> r=me if you fix the function name and the failing tests
>
> > Source/JavaScriptCore/runtime/RuntimeType.h:47
> > +RuntimeType getRuntimeTypeForValue(JSValue);
>
> We reserve the word "get" for functions returning out parameters. This
> should just be runtimeTypeForValue.
I'm waiting on a build.
I need to look into one of the failing tests. Two are just new messages, but one seems to be failing. Not sure why yet.
Created attachment 248640[details]
patch
If this goes green on bots, I'll cq+
there are a few tests that have been finicky on my machine so I'm hoping they're all good on the bots.
Created attachment 248643[details]
Archive of layout-test-results from ews101 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 248644[details]
patch
Lets give this another try.
I think one problem that is occurring is that my local machine is getting different constructor names than the bots in the error messages.
Not sure why this is.
Created attachment 248647[details]
Archive of layout-test-results from ews100 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
JoePeck helped me diagnose the problem with the bots here.
The failed tests are the output of two tests run under WK1
instead of WK2.
Not really sure what to do here since the results are divergent.
Any advice?
> Not really sure what to do here since the results are divergent.
> Any advice?
If there's a good reason for the results to diverge, then we should just check in separate test expectations for WK1 vs WK2. Is there a good reason?
(In reply to comment #63)
> > Not really sure what to do here since the results are divergent.
> > Any advice?
>
> If there's a good reason for the results to diverge, then we should just
> check in separate test expectations for WK1 vs WK2. Is there a good reason?
I need to figure out if there is a good reason or not. Off the top
of my head, my guess is that there may be divergent code paths somewhere
and one path calls into one function that creates an error and the other
calls into another function that creates an error, and those functions
create error's with different "sourceAppender"s.
I'm finally on spring break so I should have some time to look
into it this week, and inevitably, figure out how wrong my above
hypothesis is.
2015-02-24 01:31 PST, Saam Barati
2015-02-24 02:56 PST, Build Bot
2015-02-24 03:14 PST, Build Bot
2015-03-01 18:25 PST, Saam Barati
2015-03-02 15:37 PST, Saam Barati
2015-03-02 18:05 PST, Build Bot
2015-03-02 18:13 PST, Build Bot
2015-03-07 10:49 PST, Saam Barati
2015-03-10 23:18 PDT, Saam Barati
2015-03-11 10:28 PDT, Saam Barati
2015-03-11 11:47 PDT, Build Bot
2015-03-11 12:26 PDT, Build Bot
2015-03-11 19:37 PDT, Saam Barati
2015-03-11 20:14 PDT, Saam Barati
2015-03-11 21:28 PDT, Build Bot
2015-03-11 21:32 PDT, Build Bot
2015-03-12 15:10 PDT, Saam Barati
buildbot: commit-queue-
2015-03-12 16:27 PDT, Build Bot
2015-03-13 23:44 PDT, Saam Barati
2015-03-14 00:57 PDT, Build Bot
2015-03-14 01:27 PDT, Saam Barati
2015-03-14 02:36 PDT, Build Bot
2015-03-23 16:30 PDT, Saam Barati