Summary: | Annotate large string tests with largeHeap | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||
Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | darin, fpizlo, ggaren, jfbastien, keith_miller, mark.lam, msaboff, ossy, saam, sam | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Yusuke Suzuki
2017-01-14 12:33:47 PST
Created attachment 298859 [details]
Patch
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?
(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. (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 :) Created attachment 298890 [details]
Patch for landing
Committed r210775: <http://trac.webkit.org/changeset/210775> |