Analyzer doesn't get javascript overflow VM flag, so changing test to fail, ok.

Analyzer doesn't get javascript overflow VM flag, so changing test to fail, ok.

Committed: https://code.google.com/p/dart/source/detail?r=23775

[zra, 2013-06-07 21:56:55]

Committed patchset #1 manually as r23775 (presubmit successful).

[zra, 2013-06-07 21:57:08]

On 2013/06/07 21:56:55, zra wrote:
> Committed patchset #1 manually as r23775 (presubmit successful).

TBR

[Brian Wilkerson, 2013-06-07 21:59:02]

https://codereview.chromium.org/16676002/diff/1/tests/standalone/standalone.s...
File tests/standalone/standalone.status (right):

https://codereview.chromium.org/16676002/diff/1/tests/standalone/standalone.s...
tests/standalone/standalone.status:93: 53bit_overflow_literal_test: fail, ok
We were told this morning, by both Rico and Peter, that no tests (other than
co19 tests) should ever be marked as "file, ok". Please change these to "fail".

[Ivan Posva, 2013-06-07 22:16:58]

On 2013/06/07 21:59:02, Brian Wilkerson wrote:
>
https://codereview.chromium.org/16676002/diff/1/tests/standalone/standalone.s...
> File tests/standalone/standalone.status (right):
> 
>
https://codereview.chromium.org/16676002/diff/1/tests/standalone/standalone.s...
> tests/standalone/standalone.status:93: 53bit_overflow_literal_test: fail, ok
> We were told this morning, by both Rico and Peter, that no tests (other than
> co19 tests) should ever be marked as "file, ok". Please change these to
"fail".

The last time I checked "FAIL, OK" meant this is a test that we are not planning
to fix (or cannot fix) and that the failure is expected. "FAIL" in that context
meant this is a genuine failure and you are expected to fix it. There was no
communication to the contrary as far as I remember, so I advised Zach to do
exactly what he has done here. For example when running the tests with --report
I still get this status line, which confirms our understanding.
 * 35 tests are expected to fail that we won't fix

If the analyzer is going to detect and fail compilation for integer literals
that do not fit into 53-bits if the VM flag --throw_on_javascript_int_overflow,
then we should set this to fail in my understanding. If you are not planning to
fix it, which was our assumption, then "FAIL, OK" is still the correct status
according to the above reasoning.

Thanks,
-Ivan

[ahe, 2013-06-07 22:32:11]

On 2013/06/07 22:16:58, Ivan Posva wrote:
> On 2013/06/07 21:59:02, Brian Wilkerson wrote:
> >
>
https://codereview.chromium.org/16676002/diff/1/tests/standalone/standalone.s...
> > File tests/standalone/standalone.status (right):
> > 
> >
>
https://codereview.chromium.org/16676002/diff/1/tests/standalone/standalone.s...
> > tests/standalone/standalone.status:93: 53bit_overflow_literal_test: fail, ok
> > We were told this morning, by both Rico and Peter, that no tests (other than
> > co19 tests) should ever be marked as "file, ok". Please change these to
> "fail".
> 
> The last time I checked "FAIL, OK" meant this is a test that we are not
planning
> to fix (or cannot fix) and that the failure is expected. "FAIL" in that
context
> meant this is a genuine failure and you are expected to fix it. There was no
> communication to the contrary as far as I remember, so I advised Zach to do
> exactly what he has done here. For example when running the tests with
--report
> I still get this status line, which confirms our understanding.
>  * 35 tests are expected to fail that we won't fix
> 
> If the analyzer is going to detect and fail compilation for integer literals
> that do not fit into 53-bits if the VM flag
--throw_on_javascript_int_overflow,
> then we should set this to fail in my understanding. If you are not planning
to
> fix it, which was our assumption, then "FAIL, OK" is still the correct status
> according to the above reasoning.

Ivan is right.

However, the analyzer team has been misusing "Fail, OK", and we have pointed
this out on several occasions.  So earlier today, I tried to make it as simple
as possible.  Perhaps I simplified it too much.

In general, I think the analyzer team should not use "Fail, OK", and I'll be
happy to help you review CLs when you feel "Fail, OK" is appropriate.