test: strongly typed tests #1404
Conversation
| beforeEach(() => { | ||
| options.awaitWriteFinish = { pollInterval: 50, stabilityThreshold: 50 }; | ||
| options.ignoreInitial = true; | ||
|
|
||
| // Stub fs.stat() to take a while to return. | ||
| sinon.stub(fs, 'stat').callsFake((path, cb) => { | ||
| _realStat(path, w(cb, 250)); | ||
| statStub = sinon.stub(fs, 'stat').callsFake(async (path, cb) => { |
There was a problem hiding this comment.
this test can't have ever done what we thought since we moved to ESM here (i.e. since this became test.mjs)
in ESM you can't stub out a module's exports like this. which means it won't have been delaying the fs.stat calls
the test has been passing though... so maybe it just didn't actually test the thing it was trying to test? and passed because no such delay happened
for now, i skipped the test as it will throw if you try run it (sinon will throw because we try stub an ES module)
There was a problem hiding this comment.
@paulmillr what do you think we should do about this test?
im not sure we can ever stub out fs like it used to in commonjs. so we'd have to test it another way, but not sure how as i don't understand the test case enough
|
Thanks @43081j -- looks great. |
|
have published the PR now but we need to land #1403 first as this is branched off it |
Moves the tests to typescript and cleans up some things which the compiler picked up.
| beforeEach(() => { | ||
| options.awaitWriteFinish = { pollInterval: 50, stabilityThreshold: 50 }; | ||
| options.ignoreInitial = true; | ||
|
|
||
| // Stub fs.stat() to take a while to return. | ||
| sinon.stub(fs, 'stat').callsFake((path, cb) => { | ||
| _realStat(path, w(cb, 250)); | ||
| statStub = sinon.stub(fs, 'stat').callsFake(async (path, cb) => { |
|
I'll remove the disabled test in a follow-up if you don't beat me to it 👍 |
I'm leaving this as a draft for now just to show you what I was thinking @paulmillr
Basically, this moves the tests into
src/and converts them to typescript. I then ignored them from published files, etc.All seems to work but these things had to change or need discussing (if we ever want this):
node16resolution/module so*.mtsis treated as ESM andimport.metaexistsfs.Stat, but this isn't possible in ESM