Bug 167054 – Annotate large string tests with largeHeap

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

Annotate large string tests with largeHeap

[Yusuke Suzuki]

Created attachment 298859 [details]
Patch

[Darin Adler]

Comment on attachment 298859 [details]
Patch

Seems fine to me, but I am too ignorant about our testing system to be the reviewer. What problem does this fix?

[Filip Pizlo]

(In reply to comment #2)
> Comment on attachment 298859 [details]
> Patch
> 
> Seems fine to me, but I am too ignorant about our testing system to be the
> reviewer. What problem does this fix?

The test will be skipped if you --memory-limited.

The "//@" lines trigger the JSC test harness to eval that ruby statement in the context of run-jsc-stress-tests. I hadn't been aware of the largeHeap function but here it is inside run-jsc-stress-tests:

def largeHeap
    if $memoryLimited
        $didAddRunCommand = true
        puts "Skipping #{$collectionName}/#{$benchmark}"
    end
end

The only thing I would write differently here is that:

        $didAddRunCommand = true
        puts "Skipping #{$collectionName}/#{$benchmark}"

Is just the body of skip, so this should probably have been:

def largeHeap
    skip if $memoryLimited
end

But if you do that, then it's not clear if it's better for tests to say:

//@ largeHeap

or for them to say:

//@ skip if $memoryLimited

I actually somewhat prefer the latter, since it answers Darin's question immediately.  "largeHeap" does not tell me anything about the behavior while "skip if $memoryLimited" is self-explanatory.

[Yusuke Suzuki]

(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 298859 [details]
> > Patch
> > 
> > Seems fine to me, but I am too ignorant about our testing system to be the
> > reviewer. What problem does this fix?
> 
> The test will be skipped if you --memory-limited.
> 
> The "//@" lines trigger the JSC test harness to eval that ruby statement in
> the context of run-jsc-stress-tests. I hadn't been aware of the largeHeap
> function but here it is inside run-jsc-stress-tests:
> 
> def largeHeap
>     if $memoryLimited
>         $didAddRunCommand = true
>         puts "Skipping #{$collectionName}/#{$benchmark}"
>     end
> end
> 
> The only thing I would write differently here is that:
> 
>         $didAddRunCommand = true
>         puts "Skipping #{$collectionName}/#{$benchmark}"
> 
> Is just the body of skip, so this should probably have been:
> 
> def largeHeap
>     skip if $memoryLimited
> end
> 
> But if you do that, then it's not clear if it's better for tests to say:
> 
> //@ largeHeap
> 
> or for them to say:
> 
> //@ skip if $memoryLimited
> 
> I actually somewhat prefer the latter, since it answers Darin's question
> immediately.  "largeHeap" does not tell me anything about the behavior while
> "skip if $memoryLimited" is self-explanatory.

Sounds fine. So, change `// @ largeHeap` use in JSTest / LayoutTests :)

[Yusuke Suzuki]

Created attachment 298890 [details]
Patch for landing

[Yusuke Suzuki]

Committed r210775: <http://trac.webkit.org/changeset/210775>