test: increase RAM requirement for intensive tests #7772

Trott · 2016-07-17T16:41:46Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test fs

Description of change

test-fs-read-buffer-tostring-fail and test-fs-readfile-tostring-fail
have been timing out on Raspberry Pi 3 devices on the continuous
integration server. These devices have 1 Gb of RAM and the tests are
memory intensive. Previous checks for memory intensive tests used a 512
Mb cut-off, but that was probably instituted when we only had Pi 1
devices.

Consequently, this change increases the threshold for memory-intensive
tests to 1 Gb and adds that threshold to test-fs-readfile-tostring-fail.

test-fs-read-buffer-tostring-fail and test-fs-readfile-tostring-fail have been timing out on Raspberry Pi 3 devices on the continuous integration server. These devices have 1 Gb of RAM and the tests are memory intensive. Previous checks for memory intensive tests used a 512 Mb cut-off, but that was probably instituted when we only had Pi 1 devices. Consequently, this change increases the threshold for memory-intensive tests to 1 Gb and adds that threshold to test-fs-readfile-tostring-fail.

Trott · 2016-07-17T16:43:03Z

Sample CI failures this change is trying to address:

https://ci.nodejs.org/job/node-test-binary-arm/2912/RUN_SUBSET=2,label=pi3-raspbian-jessie/console:

not ok 45 parallel/test-fs-read-buffer-tostring-fail
# TIMEOUT
# (node:5468) DeprecationWarning: fs.read's legacy String interface is deprecated. Use the Buffer API as mentioned in the documentation instead.
  ---
  duration_ms: 120.147

https://ci.nodejs.org/job/node-test-binary-arm/2912/RUN_SUBSET=3,label=pi3-raspbian-jessie/console

not ok 48 parallel/test-fs-readfile-tostring-fail
# TIMEOUT
  ---
  duration_ms: 120.179

Trott · 2016-07-17T16:46:53Z

CI: https://ci.nodejs.org/job/node-test-pull-request/3322/

bnoordhuis · 2016-07-17T17:44:39Z

LGTM

Fishrock123 · 2016-07-17T19:37:47Z

Hmm, what tests on which hardware are we ignoring after this?

(I'm not comfortable about this w/o having that knowledge better available.)

cjihrig · 2016-07-18T14:43:14Z

Changes LGTM, but I'd also like to know what tests are now skipped.

Trott · 2016-07-18T22:37:11Z

Just clicked through everything in the CI results and the only thing this causes to skip the tests are the Pi 2 and Pi 3 devices. (Pi 1 devices were already skipping the tests.)

I'll rerun CI so I can confirm that it doesn't affect FreeBSD (build failure) and the plinux hosts (were hung earlier today).

Trott · 2016-07-18T22:37:47Z

CI again: https://ci.nodejs.org/job/node-test-pull-request/3332/

Trott · 2016-07-18T22:44:50Z

Here are the tests affected (but again, only on Pi devices):

$ grep -lR enoughTestMem test | grep -v test/common.js
test/addons/stringbytes-external-exceed-max/test-stringbytes-external-at-max.js
test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-ascii.js
test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-base64.js
test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-binary.js
test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-hex.js
test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-utf8.js
test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-2.js
test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max.js
test/parallel/test-fs-read-buffer-tostring-fail.js
$

This change also adds test/parallel/test-fs-readfile-tostring-fail.js to that list.

Trott · 2016-07-18T22:47:25Z

Confirmed that FreeBSD and plinux are not affected by this either. So it's just the Pi devices.

santigimeno · 2016-07-19T08:04:51Z

LGTM

cjihrig · 2016-07-19T12:38:27Z

LGTM

mhdawson · 2016-07-19T22:49:47Z

LGTM

test-fs-read-buffer-tostring-fail and test-fs-readfile-tostring-fail have been timing out on Raspberry Pi 3 devices on the continuous integration server. These devices have 1 Gb of RAM and the tests are memory intensive. Previous checks for memory intensive tests used a 512 Mb cut-off, but that was probably instituted when we only had Pi 1 devices. Consequently, this change increases the threshold for memory-intensive tests to 1 Gb and adds that threshold to test-fs-readfile-tostring-fail. PR-URL: nodejs#7772 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Trott · 2016-07-20T16:58:33Z

Landed in 135a863

Fishrock123 · 2016-07-21T07:22:28Z

@Trott Did the other tests not timeout on RPi3? If so, we should find a better solution.

test-fs-read-buffer-tostring-fail and test-fs-readfile-tostring-fail have been timing out on Raspberry Pi 3 devices on the continuous integration server. These devices have 1 Gb of RAM and the tests are memory intensive. Previous checks for memory intensive tests used a 512 Mb cut-off, but that was probably instituted when we only had Pi 1 devices. Consequently, this change increases the threshold for memory-intensive tests to 1 Gb and adds that threshold to test-fs-readfile-tostring-fail. PR-URL: #7772 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Trott · 2016-07-21T20:42:52Z

@Fishrock123 The tests were already being skipped usually but not always on Pi2 and Pi3.

I've only noticed this now and I don't have an explanation for that unless os.totalmem() is unreliable.

For example, https://ci.nodejs.org/job/node-test-binary-arm/2703/ from July 1 (before this change).

Pi 1:

ok 22 addons/stringbytes-external-exceed-max/test-stringbytes-external-at-max # skip intensive toString tests due to memory confinements
  ---
  duration_ms: 1.753
  ...
ok 23 addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max # skip intensive toString tests due to memory confinements
  ---
  duration_ms: 1.743
  ...
ok 24 addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-ascii # skip intensive toString tests due to memory confinements
  ---
  duration_ms: 1.747
  ...
ok 25 addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-base64 # skip intensive toString tests due to memory confinements
  ---
  duration_ms: 1.748
  ...
ok 26 addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-binary # skip intensive toString tests due to memory confinements
  ---
  duration_ms: 1.854
  ...
ok 27 addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-hex # skip intensive toString tests due to memory confinements
  ---
  duration_ms: 1.748
  ...
ok 28 addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-utf8 # skip intensive toString tests due to memory confinements
  ---
  duration_ms: 1.750
  ...
ok 29 addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-2 # skip intensive toString tests due to memory confinements
  ---
  duration_ms: 1.747

OK, that makes sense. It skips all those tests on the Pi 1 which has only 512 Mb or something like that.

Let's check the Pi 2 run:

ok 22 addons/stringbytes-external-exceed-max/test-stringbytes-external-at-max
  ---
  duration_ms: 2.135
  ...
ok 23 addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max # skip intensive toString tests due to memory confinements
  ---
  duration_ms: 1.31
  ...
ok 24 addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-ascii # skip intensive toString tests due to memory confinements
  ---
  duration_ms: 1.35
  ...
ok 25 addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-base64 # skip intensive toString tests due to memory confinements
  ---
  duration_ms: 1.35
  ...
ok 26 addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-binary
  ---
  duration_ms: 4.342
  ...
ok 27 addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-hex
  ---
  duration_ms: 8.453
  ...
ok 28 addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-utf8
  ---
  duration_ms: 1.29
  ...
ok 29 addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-2
  ---
  duration_ms: 2.32

Uh....OK, it skips some of those tests but not all of them. So os.totalmem() is returning different values on different instantiations of node processes on the same hardware? ????

ANYWAY, if someone wants to come up with a more nuanced way to handle these resource-intensive tests, I'm open to it. The trade-off will be complexity. The benefit of the existing methodology is simplicity. (I did think about making it a function and having each test pass the RAM threshold in bytes as an argument, but at that point, you start injecting different magic numbers into different tests. Blech. At least this way, there's only one magic number and it's in common.js only. Trade off for the simplicity is a lack of flexibility. I'm OK with that, personally.)

test-fs-read-buffer-tostring-fail and test-fs-readfile-tostring-fail have been timing out on Raspberry Pi 3 devices on the continuous integration server. These devices have 1 Gb of RAM and the tests are memory intensive. Previous checks for memory intensive tests used a 512 Mb cut-off, but that was probably instituted when we only had Pi 1 devices. Consequently, this change increases the threshold for memory-intensive tests to 1 Gb and adds that threshold to test-fs-readfile-tostring-fail. PR-URL: #7772 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Trott added fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests. labels Jul 17, 2016

Trott mentioned this pull request Jul 20, 2016

deps: cherry-pick 1f53e42 from v8 upstream (v6.x) #7789

Closed

Trott closed this Jul 20, 2016

evanlucas mentioned this pull request Jul 21, 2016

v6.3.1 Release Proposal #7782

Merged

Trott added the lts-watch-v4.x label Sep 9, 2016

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Oct 10, 2016

MylesBorins mentioned this pull request Oct 26, 2016

V4.6.2 proposal #9298

Closed

Trott deleted the mem branch January 13, 2022 22:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: increase RAM requirement for intensive tests #7772

test: increase RAM requirement for intensive tests #7772

Trott commented Jul 17, 2016

Trott commented Jul 17, 2016

Trott commented Jul 17, 2016

bnoordhuis commented Jul 17, 2016

Fishrock123 commented Jul 17, 2016 •

edited

Loading

cjihrig commented Jul 18, 2016

Trott commented Jul 18, 2016

Trott commented Jul 18, 2016

Trott commented Jul 18, 2016

Trott commented Jul 18, 2016

santigimeno commented Jul 19, 2016

cjihrig commented Jul 19, 2016

mhdawson commented Jul 19, 2016

Trott commented Jul 20, 2016

Fishrock123 commented Jul 21, 2016

Trott commented Jul 21, 2016 •

edited

Loading

test: increase RAM requirement for intensive tests #7772

test: increase RAM requirement for intensive tests #7772

Conversation

Trott commented Jul 17, 2016

Checklist

Affected core subsystem(s)

Description of change

Trott commented Jul 17, 2016

Trott commented Jul 17, 2016

bnoordhuis commented Jul 17, 2016

Fishrock123 commented Jul 17, 2016 • edited Loading

cjihrig commented Jul 18, 2016

Trott commented Jul 18, 2016

Trott commented Jul 18, 2016

Trott commented Jul 18, 2016

Trott commented Jul 18, 2016

santigimeno commented Jul 19, 2016

cjihrig commented Jul 19, 2016

mhdawson commented Jul 19, 2016

Trott commented Jul 20, 2016

Fishrock123 commented Jul 21, 2016

Trott commented Jul 21, 2016 • edited Loading

Fishrock123 commented Jul 17, 2016 •

edited

Loading

Trott commented Jul 21, 2016 •

edited

Loading