Possibly fix flaky test seen to error in wasm2js3.test_stat_chmod_wasmfs. by juj · Pull Request #24989 · emscripten-core/emscripten

juj · 2025-08-20T20:28:16Z

time_t is long long, test has a narrowing conversion to int.

Fails nondeterministically in http://clbri.com:8010/api/v2/logs/20165/raw_inline

sbc100

It seems likely this is a timing issue since its rely on sleep(1) and assumes the file created/touched before after the sleep will have different timestamps.

I had thought we had a bug open about this test already but I don't see one.

Perhaps you could open a bug and mark this test with @flaky(...bug url...).. that way it should get re-tried automatically in CI (see EMTEST_RETRY_FLAKY, you probably want to set that to a non-zero value in your CI too).

sbc100 · 2025-08-20T20:35:34Z

I found the existing issue : #24905

juj · 2025-08-20T20:44:04Z

Could be, hence the "possibly fix."

Nevertheless, long long shouldn't be narrowed to int, so this change is correct. If there's a filesystem that actually has 64-bit timestamps, then the code would be incorrect there. After PR #17401 this test wasn't adapted accordingly.

juj · 2025-08-20T20:44:52Z

Perhaps you could open a bug and mark this test with @flaky(...bug url...).. that way it should get re-tried automatically in CI (see EMTEST_RETRY_FLAKY, you probably want to set that to a non-zero value in your CI too).

Separate PR.

sbc100 · 2025-08-20T20:58:28Z

 if nodefs and self.get_setting('WASMFS'):
 self.skipTest('test requires symlink creation which currently missing from wasmfs+noderawfs')
- self.do_runf('stat/test_chmod.c', 'success')
+ self.do_runf('stat/test_chmod.c', 'success', cflags=['-Werror=conversion'])


Makes me wonder if we should make an effort to enable this in whole test suite.
We currently enable -Werror by default:
emscripten/test/common.py
Line 1202 in 4cab039
self.cflags= ['-Wclosure', '-Werror', '-Wno-limited-postlink-optimizations']
But we don't enable -Wall (which looks like it doesn't actually include -Wconversion anyway )

I was expecting you might suggest that, and did give it a quick test before this PR, and was met with a million double -> float narrowing warnings with most tests failing miserably. It would be a nice idea, something to work on later.
I also searched the repo for other uses of .st_mtime and this was the only occurrence that assigned that to an int, other places have been converted already before.

Yeah, no worries, I think we are long way from -Wall or -Wextra by default in the test suite !

…mfs. (emscripten-core#24989) `time_t` is `long long`, test has a narrowing conversion to `int`. Fails nondeterministically in http://clbri.com:8010/api/v2/logs/20165/raw_inline

juj · 2025-08-20T20:28:16Z

time_t is long long, test has a narrowing conversion to int.

Fails nondeterministically in http://clbri.com:8010/api/v2/logs/20165/raw_inline

sbc100

It seems likely this is a timing issue since its rely on sleep(1) and assumes the file created/touched before after the sleep will have different timestamps.

I had thought we had a bug open about this test already but I don't see one.

Perhaps you could open a bug and mark this test with @flaky(...bug url...).. that way it should get re-tried automatically in CI (see EMTEST_RETRY_FLAKY, you probably want to set that to a non-zero value in your CI too).

sbc100 · 2025-08-20T20:35:34Z

I found the existing issue : #24905

juj · 2025-08-20T20:44:04Z

Could be, hence the "possibly fix."

Nevertheless, long long shouldn't be narrowed to int, so this change is correct. If there's a filesystem that actually has 64-bit timestamps, then the code would be incorrect there. After PR #17401 this test wasn't adapted accordingly.

juj · 2025-08-20T20:44:52Z

Perhaps you could open a bug and mark this test with @flaky(...bug url...).. that way it should get re-tried automatically in CI (see EMTEST_RETRY_FLAKY, you probably want to set that to a non-zero value in your CI too).

Separate PR.

sbc100 · 2025-08-20T20:58:28Z

 if nodefs and self.get_setting('WASMFS'):
 self.skipTest('test requires symlink creation which currently missing from wasmfs+noderawfs')
- self.do_runf('stat/test_chmod.c', 'success')
+ self.do_runf('stat/test_chmod.c', 'success', cflags=['-Werror=conversion'])


Makes me wonder if we should make an effort to enable this in whole test suite.
We currently enable -Werror by default:
emscripten/test/common.py
Line 1202 in 4cab039
self.cflags= ['-Wclosure', '-Werror', '-Wno-limited-postlink-optimizations']
But we don't enable -Wall (which looks like it doesn't actually include -Wconversion anyway )

I was expecting you might suggest that, and did give it a quick test before this PR, and was met with a million double -> float narrowing warnings with most tests failing miserably. It would be a nice idea, something to work on later.
I also searched the repo for other uses of .st_mtime and this was the only occurrence that assigned that to an int, other places have been converted already before.

Yeah, no worries, I think we are long way from -Wall or -Wextra by default in the test suite !

…mfs. (emscripten-core#24989) `time_t` is `long long`, test has a narrowing conversion to `int`. Fails nondeterministically in http://clbri.com:8010/api/v2/logs/20165/raw_inline

juj · 2025-08-20T20:28:16Z

time_t is long long, test has a narrowing conversion to int.

Fails nondeterministically in http://clbri.com:8010/api/v2/logs/20165/raw_inline

sbc100

It seems likely this is a timing issue since its rely on sleep(1) and assumes the file created/touched before after the sleep will have different timestamps.

I had thought we had a bug open about this test already but I don't see one.

Perhaps you could open a bug and mark this test with @flaky(...bug url...).. that way it should get re-tried automatically in CI (see EMTEST_RETRY_FLAKY, you probably want to set that to a non-zero value in your CI too).

sbc100 · 2025-08-20T20:35:34Z

I found the existing issue : #24905

juj · 2025-08-20T20:44:04Z

Could be, hence the "possibly fix."

Nevertheless, long long shouldn't be narrowed to int, so this change is correct. If there's a filesystem that actually has 64-bit timestamps, then the code would be incorrect there. After PR #17401 this test wasn't adapted accordingly.

juj · 2025-08-20T20:44:52Z

Perhaps you could open a bug and mark this test with @flaky(...bug url...).. that way it should get re-tried automatically in CI (see EMTEST_RETRY_FLAKY, you probably want to set that to a non-zero value in your CI too).

Separate PR.

sbc100 · 2025-08-20T20:58:28Z

 if nodefs and self.get_setting('WASMFS'):
 self.skipTest('test requires symlink creation which currently missing from wasmfs+noderawfs')
- self.do_runf('stat/test_chmod.c', 'success')
+ self.do_runf('stat/test_chmod.c', 'success', cflags=['-Werror=conversion'])


Makes me wonder if we should make an effort to enable this in whole test suite.
We currently enable -Werror by default:
emscripten/test/common.py
Line 1202 in 4cab039
self.cflags= ['-Wclosure', '-Werror', '-Wno-limited-postlink-optimizations']
But we don't enable -Wall (which looks like it doesn't actually include -Wconversion anyway )

I was expecting you might suggest that, and did give it a quick test before this PR, and was met with a million double -> float narrowing warnings with most tests failing miserably. It would be a nice idea, something to work on later.
I also searched the repo for other uses of .st_mtime and this was the only occurrence that assigned that to an int, other places have been converted already before.

Yeah, no worries, I think we are long way from -Wall or -Wextra by default in the test suite !

…mfs. (emscripten-core#24989) `time_t` is `long long`, test has a narrowing conversion to `int`. Fails nondeterministically in http://clbri.com:8010/api/v2/logs/20165/raw_inline

juj · 2025-08-20T20:28:16Z

time_t is long long, test has a narrowing conversion to int.

Fails nondeterministically in http://clbri.com:8010/api/v2/logs/20165/raw_inline

sbc100

It seems likely this is a timing issue since its rely on sleep(1) and assumes the file created/touched before after the sleep will have different timestamps.

I had thought we had a bug open about this test already but I don't see one.

Perhaps you could open a bug and mark this test with @flaky(...bug url...).. that way it should get re-tried automatically in CI (see EMTEST_RETRY_FLAKY, you probably want to set that to a non-zero value in your CI too).

sbc100 · 2025-08-20T20:35:34Z

I found the existing issue : #24905

juj · 2025-08-20T20:44:04Z

Could be, hence the "possibly fix."

Nevertheless, long long shouldn't be narrowed to int, so this change is correct. If there's a filesystem that actually has 64-bit timestamps, then the code would be incorrect there. After PR #17401 this test wasn't adapted accordingly.

juj · 2025-08-20T20:44:52Z

Perhaps you could open a bug and mark this test with @flaky(...bug url...).. that way it should get re-tried automatically in CI (see EMTEST_RETRY_FLAKY, you probably want to set that to a non-zero value in your CI too).

Separate PR.

sbc100 · 2025-08-20T20:58:28Z

 if nodefs and self.get_setting('WASMFS'):
 self.skipTest('test requires symlink creation which currently missing from wasmfs+noderawfs')
- self.do_runf('stat/test_chmod.c', 'success')
+ self.do_runf('stat/test_chmod.c', 'success', cflags=['-Werror=conversion'])


Makes me wonder if we should make an effort to enable this in whole test suite.
We currently enable -Werror by default:
emscripten/test/common.py
Line 1202 in 4cab039
self.cflags= ['-Wclosure', '-Werror', '-Wno-limited-postlink-optimizations']
But we don't enable -Wall (which looks like it doesn't actually include -Wconversion anyway )

I was expecting you might suggest that, and did give it a quick test before this PR, and was met with a million double -> float narrowing warnings with most tests failing miserably. It would be a nice idea, something to work on later.
I also searched the repo for other uses of .st_mtime and this was the only occurrence that assigned that to an int, other places have been converted already before.

Yeah, no worries, I think we are long way from -Wall or -Wextra by default in the test suite !

…mfs. (emscripten-core#24989) `time_t` is `long long`, test has a narrowing conversion to `int`. Fails nondeterministically in http://clbri.com:8010/api/v2/logs/20165/raw_inline

juj · 2025-08-20T20:28:16Z

time_t is long long, test has a narrowing conversion to int.

Fails nondeterministically in http://clbri.com:8010/api/v2/logs/20165/raw_inline

sbc100

It seems likely this is a timing issue since its rely on sleep(1) and assumes the file created/touched before after the sleep will have different timestamps.

I had thought we had a bug open about this test already but I don't see one.

Perhaps you could open a bug and mark this test with @flaky(...bug url...).. that way it should get re-tried automatically in CI (see EMTEST_RETRY_FLAKY, you probably want to set that to a non-zero value in your CI too).

sbc100 · 2025-08-20T20:35:34Z

I found the existing issue : #24905

juj · 2025-08-20T20:44:04Z

Could be, hence the "possibly fix."

Nevertheless, long long shouldn't be narrowed to int, so this change is correct. If there's a filesystem that actually has 64-bit timestamps, then the code would be incorrect there. After PR #17401 this test wasn't adapted accordingly.

juj · 2025-08-20T20:44:52Z

Perhaps you could open a bug and mark this test with @flaky(...bug url...).. that way it should get re-tried automatically in CI (see EMTEST_RETRY_FLAKY, you probably want to set that to a non-zero value in your CI too).

Separate PR.

sbc100 · 2025-08-20T20:58:28Z

 if nodefs and self.get_setting('WASMFS'):
 self.skipTest('test requires symlink creation which currently missing from wasmfs+noderawfs')
- self.do_runf('stat/test_chmod.c', 'success')
+ self.do_runf('stat/test_chmod.c', 'success', cflags=['-Werror=conversion'])


Makes me wonder if we should make an effort to enable this in whole test suite.
We currently enable -Werror by default:
emscripten/test/common.py
Line 1202 in 4cab039
self.cflags= ['-Wclosure', '-Werror', '-Wno-limited-postlink-optimizations']
But we don't enable -Wall (which looks like it doesn't actually include -Wconversion anyway )

I was expecting you might suggest that, and did give it a quick test before this PR, and was met with a million double -> float narrowing warnings with most tests failing miserably. It would be a nice idea, something to work on later.
I also searched the repo for other uses of .st_mtime and this was the only occurrence that assigned that to an int, other places have been converted already before.

Yeah, no worries, I think we are long way from -Wall or -Wextra by default in the test suite !

…mfs. (emscripten-core#24989) `time_t` is `long long`, test has a narrowing conversion to `int`. Fails nondeterministically in http://clbri.com:8010/api/v2/logs/20165/raw_inline

juj · 2025-08-20T20:28:16Z

time_t is long long, test has a narrowing conversion to int.

Fails nondeterministically in http://clbri.com:8010/api/v2/logs/20165/raw_inline

sbc100

It seems likely this is a timing issue since its rely on sleep(1) and assumes the file created/touched before after the sleep will have different timestamps.

I had thought we had a bug open about this test already but I don't see one.

Perhaps you could open a bug and mark this test with @flaky(...bug url...).. that way it should get re-tried automatically in CI (see EMTEST_RETRY_FLAKY, you probably want to set that to a non-zero value in your CI too).

sbc100 · 2025-08-20T20:35:34Z

I found the existing issue : #24905

juj · 2025-08-20T20:44:04Z

Could be, hence the "possibly fix."

Nevertheless, long long shouldn't be narrowed to int, so this change is correct. If there's a filesystem that actually has 64-bit timestamps, then the code would be incorrect there. After PR #17401 this test wasn't adapted accordingly.

juj · 2025-08-20T20:44:52Z

Perhaps you could open a bug and mark this test with @flaky(...bug url...).. that way it should get re-tried automatically in CI (see EMTEST_RETRY_FLAKY, you probably want to set that to a non-zero value in your CI too).

Separate PR.

sbc100 · 2025-08-20T20:58:28Z

 if nodefs and self.get_setting('WASMFS'):
 self.skipTest('test requires symlink creation which currently missing from wasmfs+noderawfs')
- self.do_runf('stat/test_chmod.c', 'success')
+ self.do_runf('stat/test_chmod.c', 'success', cflags=['-Werror=conversion'])


Makes me wonder if we should make an effort to enable this in whole test suite.
We currently enable -Werror by default:
emscripten/test/common.py
Line 1202 in 4cab039
self.cflags= ['-Wclosure', '-Werror', '-Wno-limited-postlink-optimizations']
But we don't enable -Wall (which looks like it doesn't actually include -Wconversion anyway )

I was expecting you might suggest that, and did give it a quick test before this PR, and was met with a million double -> float narrowing warnings with most tests failing miserably. It would be a nice idea, something to work on later.
I also searched the repo for other uses of .st_mtime and this was the only occurrence that assigned that to an int, other places have been converted already before.

Yeah, no worries, I think we are long way from -Wall or -Wextra by default in the test suite !

…mfs. (emscripten-core#24989) `time_t` is `long long`, test has a narrowing conversion to `int`. Fails nondeterministically in http://clbri.com:8010/api/v2/logs/20165/raw_inline

juj · 2025-08-20T20:28:16Z

time_t is long long, test has a narrowing conversion to int.

Fails nondeterministically in http://clbri.com:8010/api/v2/logs/20165/raw_inline

sbc100

It seems likely this is a timing issue since its rely on sleep(1) and assumes the file created/touched before after the sleep will have different timestamps.

I had thought we had a bug open about this test already but I don't see one.

Perhaps you could open a bug and mark this test with @flaky(...bug url...).. that way it should get re-tried automatically in CI (see EMTEST_RETRY_FLAKY, you probably want to set that to a non-zero value in your CI too).

sbc100 · 2025-08-20T20:35:34Z

I found the existing issue : #24905

juj · 2025-08-20T20:44:04Z

Could be, hence the "possibly fix."

Nevertheless, long long shouldn't be narrowed to int, so this change is correct. If there's a filesystem that actually has 64-bit timestamps, then the code would be incorrect there. After PR #17401 this test wasn't adapted accordingly.

juj · 2025-08-20T20:44:52Z

Perhaps you could open a bug and mark this test with @flaky(...bug url...).. that way it should get re-tried automatically in CI (see EMTEST_RETRY_FLAKY, you probably want to set that to a non-zero value in your CI too).

Separate PR.

sbc100 · 2025-08-20T20:58:28Z

 if nodefs and self.get_setting('WASMFS'):
 self.skipTest('test requires symlink creation which currently missing from wasmfs+noderawfs')
- self.do_runf('stat/test_chmod.c', 'success')
+ self.do_runf('stat/test_chmod.c', 'success', cflags=['-Werror=conversion'])


Makes me wonder if we should make an effort to enable this in whole test suite.
We currently enable -Werror by default:
emscripten/test/common.py
Line 1202 in 4cab039
self.cflags= ['-Wclosure', '-Werror', '-Wno-limited-postlink-optimizations']
But we don't enable -Wall (which looks like it doesn't actually include -Wconversion anyway )

I was expecting you might suggest that, and did give it a quick test before this PR, and was met with a million double -> float narrowing warnings with most tests failing miserably. It would be a nice idea, something to work on later.
I also searched the repo for other uses of .st_mtime and this was the only occurrence that assigned that to an int, other places have been converted already before.

Yeah, no worries, I think we are long way from -Wall or -Wextra by default in the test suite !

…mfs. (emscripten-core#24989) `time_t` is `long long`, test has a narrowing conversion to `int`. Fails nondeterministically in http://clbri.com:8010/api/v2/logs/20165/raw_inline

juj · 2025-08-20T20:28:16Z

time_t is long long, test has a narrowing conversion to int.

Fails nondeterministically in http://clbri.com:8010/api/v2/logs/20165/raw_inline

sbc100

It seems likely this is a timing issue since its rely on sleep(1) and assumes the file created/touched before after the sleep will have different timestamps.

I had thought we had a bug open about this test already but I don't see one.

Perhaps you could open a bug and mark this test with @flaky(...bug url...).. that way it should get re-tried automatically in CI (see EMTEST_RETRY_FLAKY, you probably want to set that to a non-zero value in your CI too).

sbc100 · 2025-08-20T20:35:34Z

I found the existing issue : #24905

juj · 2025-08-20T20:44:04Z

Could be, hence the "possibly fix."

Nevertheless, long long shouldn't be narrowed to int, so this change is correct. If there's a filesystem that actually has 64-bit timestamps, then the code would be incorrect there. After PR #17401 this test wasn't adapted accordingly.

juj · 2025-08-20T20:44:52Z

Perhaps you could open a bug and mark this test with @flaky(...bug url...).. that way it should get re-tried automatically in CI (see EMTEST_RETRY_FLAKY, you probably want to set that to a non-zero value in your CI too).

Separate PR.

sbc100 · 2025-08-20T20:58:28Z

 if nodefs and self.get_setting('WASMFS'):
 self.skipTest('test requires symlink creation which currently missing from wasmfs+noderawfs')
- self.do_runf('stat/test_chmod.c', 'success')
+ self.do_runf('stat/test_chmod.c', 'success', cflags=['-Werror=conversion'])


Makes me wonder if we should make an effort to enable this in whole test suite.
We currently enable -Werror by default:
emscripten/test/common.py
Line 1202 in 4cab039
self.cflags= ['-Wclosure', '-Werror', '-Wno-limited-postlink-optimizations']
But we don't enable -Wall (which looks like it doesn't actually include -Wconversion anyway )

I was expecting you might suggest that, and did give it a quick test before this PR, and was met with a million double -> float narrowing warnings with most tests failing miserably. It would be a nice idea, something to work on later.
I also searched the repo for other uses of .st_mtime and this was the only occurrence that assigned that to an int, other places have been converted already before.

Yeah, no worries, I think we are long way from -Wall or -Wextra by default in the test suite !

…mfs. (emscripten-core#24989) `time_t` is `long long`, test has a narrowing conversion to `int`. Fails nondeterministically in http://clbri.com:8010/api/v2/logs/20165/raw_inline

Fix flaky test seen to error in wasm2js3.test_stat_chmod_wasmfs.
4aa2673