Issue 3593 - stop sharing v8::Isolate among tests

# The diff seems correct but side-by-side view is broken, and any attempt to submit it fails.

[hub, 2017-05-19 19:20:21]

https://codereview.adblockplus.org/29442722/diff/29442723/test/JsEngine.cpp
File test/JsEngine.cpp (right):

https://codereview.adblockplus.org/29442722/diff/29442723/test/JsEngine.cpp#n...
test/JsEngine.cpp:206: 
I am not convinced by this test. We run mostly on 64-bits on our development
machine (Linux or macOS at least) where the address space is larger.

https://codereview.adblockplus.org/29442722/diff/29442723/test/JsEngine.cpp#n...
test/JsEngine.cpp:208: 
Shouldn't this be written 
  while (i-- > 0)
?

Also I'd rather see a
  for (int i = 0; i < 1000; i++)

but this is strict a matter of preference.

[sergei, 2017-05-22 08:08:55]

https://codereview.adblockplus.org/29442722/diff/29442723/test/JsEngine.cpp
File test/JsEngine.cpp (right):

https://codereview.adblockplus.org/29442722/diff/29442723/test/JsEngine.cpp#n...
test/JsEngine.cpp:206: 
On 2017/05/19 19:20:21, hub wrote:
> I am not convinced by this test. We run mostly on 64-bits on our development
> machine (Linux or macOS at least) where the address space is larger.

On 64 bit platform it will take too long to try to run out of memory, so it's
just a simple attempt to catch when we do something wrong instead of nothing at
all, do you have some alternative?

BTW, I'm not sure that we mostly run on 64-bits because currently targeted by us
platforms are Windows (32 and 64 bits) and Android (where we use only 32 bits
version). I agree that in a long term 32 bit will be likely thrown away but it
seems it won't happen withing nearest couple years.

https://codereview.adblockplus.org/29442722/diff/29442723/test/JsEngine.cpp#n...
test/JsEngine.cpp:208: 
On 2017/05/19 19:20:21, hub wrote:
> Shouldn't this be written 
>   while (i-- > 0)
> ?
> 
> Also I'd rather see a
>   for (int i = 0; i < 1000; i++)
> 
> but this is strict a matter of preference.

Done.

[hub, 2017-05-23 14:08:13]

On 2017/05/22 08:08:55, sergei wrote:
> https://codereview.adblockplus.org/29442722/diff/29442723/test/JsEngine.cpp
> File test/JsEngine.cpp (right):
> 
>
https://codereview.adblockplus.org/29442722/diff/29442723/test/JsEngine.cpp#n...
> test/JsEngine.cpp:206: 
> On 2017/05/19 19:20:21, hub wrote:
> > I am not convinced by this test. We run mostly on 64-bits on our development
> > machine (Linux or macOS at least) where the address space is larger.
> 
> On 64 bit platform it will take too long to try to run out of memory, so it's
> just a simple attempt to catch when we do something wrong instead of nothing
at
> all, do you have some alternative?

In any platform building with clang or gcc (Linux/Android, macOS), there is the
addresss sanitizer that does (amongst other things) leak detection. Can't speak
about Windows though.

> BTW, I'm not sure that we mostly run on 64-bits because currently targeted by
us
> platforms are Windows (32 and 64 bits) and Android (where we use only 32 bits
> version). I agree that in a long term 32 bit will be likely thrown away but it
> seems it won't happen withing nearest couple years.

We run the tests on our development machine - and here it is all x86_64 (either
macOS, or my personal Linux system). While it is not 100% full proof since the
actual target platform can trigger some other bugs, it definitely is worth it. I
would consider make that test "32-bits" only and marking as skipped on arch with
a larger address space. This would clarify at least the intent.

[sergei, 2017-05-24 16:22:32]

https://codereview.adblockplus.org/29442722/diff/29447563/test/JsEngine.cpp
File test/JsEngine.cpp (right):

https://codereview.adblockplus.org/29442722/diff/29447563/test/JsEngine.cpp#n...
test/JsEngine.cpp:200: #if UINTPTR_MAX == UINT32_MAX // detection of 32-bit
platform
Although it might seem hacky, on practice it turned out the most reliable
cross-complier way to detect 32 bit platform.

[hub, 2017-05-24 16:31:40]

https://codereview.adblockplus.org/29442722/diff/29447563/test/JsEngine.cpp
File test/JsEngine.cpp (right):

https://codereview.adblockplus.org/29442722/diff/29447563/test/JsEngine.cpp#n...
test/JsEngine.cpp:200: #if UINTPTR_MAX == UINT32_MAX // detection of 32-bit
platform
On 2017/05/24 16:22:32, sergei wrote:
> Although it might seem hacky, on practice it turned out the most reliable
> cross-complier way to detect 32 bit platform.

We could do something like 

#if UINTPTR_MAX == UINT32_MAX // detection of 32-bit platform
TEST(NewJsEngineTest, MemoryLeak_NoLeak)
#else
TEST(NewJsEngineTest, DISABLED_32bitsOnly_MemoryLeak_NoLeak)
#endif

This would make the test disabled, but still compiled, and state in the output
why it is disabled.

[sergei, 2017-05-24 17:05:51]

https://codereview.adblockplus.org/29442722/diff/29447563/test/JsEngine.cpp
File test/JsEngine.cpp (right):

https://codereview.adblockplus.org/29442722/diff/29447563/test/JsEngine.cpp#n...
test/JsEngine.cpp:200: #if UINTPTR_MAX == UINT32_MAX // detection of 32-bit
platform
On 2017/05/24 16:31:40, hub wrote:
> On 2017/05/24 16:22:32, sergei wrote:
> > Although it might seem hacky, on practice it turned out the most reliable
> > cross-complier way to detect 32 bit platform.
> 
> We could do something like 
> 
> #if UINTPTR_MAX == UINT32_MAX // detection of 32-bit platform
> TEST(NewJsEngineTest, MemoryLeak_NoLeak)
> #else
> TEST(NewJsEngineTest, DISABLED_32bitsOnly_MemoryLeak_NoLeak)
> #endif
> 
> This would make the test disabled, but still compiled, and state in the output
> why it is disabled.
Since in this case it will still run, so not disabled, I would rather replace
DISABLED by something else, like VALID or even drop it completely,
32bitsOnly_MemoryLeak_NoLeak. I am not sure that it makes sense to rename the
test depending on the platform, so #if ...#endif can be removed. What about
putting it at the end, MemoryLeak_NoLeak_32bitsOnly?
Thoughts?

[hub, 2017-05-24 18:18:07]

https://codereview.adblockplus.org/29442722/diff/29447563/test/JsEngine.cpp
File test/JsEngine.cpp (right):

https://codereview.adblockplus.org/29442722/diff/29447563/test/JsEngine.cpp#n...
test/JsEngine.cpp:200: #if UINTPTR_MAX == UINT32_MAX // detection of 32-bit
platform
On 2017/05/24 17:05:51, sergei wrote:
> On 2017/05/24 16:31:40, hub wrote:
> > On 2017/05/24 16:22:32, sergei wrote:
> > > Although it might seem hacky, on practice it turned out the most reliable
> > > cross-complier way to detect 32 bit platform.
> > 
> > We could do something like 
> > 
> > #if UINTPTR_MAX == UINT32_MAX // detection of 32-bit platform
> > TEST(NewJsEngineTest, MemoryLeak_NoLeak)
> > #else
> > TEST(NewJsEngineTest, DISABLED_32bitsOnly_MemoryLeak_NoLeak)
> > #endif
> > 
> > This would make the test disabled, but still compiled, and state in the
output
> > why it is disabled.
> Since in this case it will still run, so not disabled, I would rather replace
> DISABLED by something else, like VALID or even drop it completely,
> 32bitsOnly_MemoryLeak_NoLeak. I am not sure that it makes sense to rename the
> test depending on the platform, so #if ...#endif can be removed. What about
> putting it at the end, MemoryLeak_NoLeak_32bitsOnly?
> Thoughts?

According to this, the test will be disabled (build still comnpiled):

https://github.com/google/googletest/blob/master/googletest/docs/V1_7_Advance...

Since we can't really output any message I find that changing the test name
would help, but if you prefer not, it's fine by me.

[sergei, 2017-05-26 10:56:43]

https://codereview.adblockplus.org/29442722/diff/29447563/test/JsEngine.cpp
File test/JsEngine.cpp (right):

https://codereview.adblockplus.org/29442722/diff/29447563/test/JsEngine.cpp#n...
test/JsEngine.cpp:200: #if UINTPTR_MAX == UINT32_MAX // detection of 32-bit
platform
On 2017/05/24 18:18:07, hub wrote:
> On 2017/05/24 17:05:51, sergei wrote:
> > On 2017/05/24 16:31:40, hub wrote:
> > > On 2017/05/24 16:22:32, sergei wrote:
> > > > Although it might seem hacky, on practice it turned out the most
reliable
> > > > cross-complier way to detect 32 bit platform.
> > > 
> > > We could do something like 
> > > 
> > > #if UINTPTR_MAX == UINT32_MAX // detection of 32-bit platform
> > > TEST(NewJsEngineTest, MemoryLeak_NoLeak)
> > > #else
> > > TEST(NewJsEngineTest, DISABLED_32bitsOnly_MemoryLeak_NoLeak)
> > > #endif
> > > 
> > > This would make the test disabled, but still compiled, and state in the
> output
> > > why it is disabled.
> > Since in this case it will still run, so not disabled, I would rather
replace
> > DISABLED by something else, like VALID or even drop it completely,
> > 32bitsOnly_MemoryLeak_NoLeak. I am not sure that it makes sense to rename
the
> > test depending on the platform, so #if ...#endif can be removed. What about
> > putting it at the end, MemoryLeak_NoLeak_32bitsOnly?
> > Thoughts?
> 
> According to this, the test will be disabled (build still comnpiled):
> 
>
https://github.com/google/googletest/blob/master/googletest/docs/V1_7_Advance...
> 
> Since we can't really output any message I find that changing the test name
> would help, but if you prefer not, it's fine by me.

That's a cool feature of gtest I was not aware of. What about the new variant?

[sergei, 2017-05-26 11:12:55]

BTW, I'm also preparing changes with updating of v8 for uploading and had to
disable this test for debug for 32 bits because it takes 20-25 seconds to
instantiate v8::isolate in debug mode on windows (even without debug heap) and
therefore it takes ages (actually longer than 20 000 seconds = 5 hours) to run
this single test :).

[hub, 2017-05-26 12:01:25]

LGTM