The tests failed because the renameSyncErrorsUnix will check the error message, change fs_err::rename back to std::fs::rename separately will pass the test, but It may be a little weird that only rename uses the method in the std::fs, so may be we can change the renameSyncErrorsUnix test to let the code pass or write the error message for every fs method manually, any suggestions? @bartlomieju @F3n67u

The tests failed because the renameSyncErrorsUnix will check the error message, change fs_err::rename back to std::fs::rename separately will pass the test, but It may be a little weird that only rename uses the method in the std::fs, so may be we can change the renameSyncErrorsUnix test to let the code pass or write the error message for every fs method manually, any suggestions? @bartlomieju @F3n67u

I'll prefer the latter approach: write the error message for every fs method manually.

The tests failed because the renameSyncErrorsUnix will check the error message, change fs_err::rename back to std::fs::rename separately will pass the test, but It may be a little weird that only rename uses the method in the std::fs, so may be we can change the renameSyncErrorsUnix test to let the code pass or write the error message for every fs method manually, any suggestions? @bartlomieju @F3n67u

I'll prefer the latter approach: write the error message for every fs method manually.

I agree, message should be written manually.

@bartlomieju I already add path info to all std::fs call's error in runtime fs. Could you help to review?

I don't known how to add path info to permissions.read.check(&path)? and permissions.write.check(&path)? bec those method return anyhow::Error instead of std::io::Error(I don't know how to add path info to anyhow::Error and make path info print). I will very appreciate if you can help with this problem.

@bartlomieju I already add path info to all std::fs call's error in runtime fs. Could you help to review?

The PR looks good! I'm sure there will be some bike-shedding regarding what errors should print exactly, but it's not a big deal.

I don't known how to add path info to permissions.read.check(&path)? and permissions.write.check(&path)? bec those method return anyhow::Error instead of std::io::Error(I don't know how to add path info to anyhow::Error and make path info print). I will very appreciate if you can help with this problem.

Permission errors coming from read.check() and write.check() already have paths printed out so there's no need to do anything there.

Would be great to see some updated to the unit tests that call assertThrows or assertRejects and verify that expected filepaths are included in the error message.

@dsherret do you have any suggestion as for the error messages?

@F3n67u please add some tests asserting error message contain paths so we can land this PR.

@bartlomieju nope, it looks good to me other than the tests.

@bartlomieju I already add path info to all std::fs call's error in runtime fs. Could you help to review?

The PR looks good! I'm sure there will be some bike-shedding regarding what errors should print exactly, but it's not a big deal.

I don't known how to add path info to permissions.read.check(&path)? and permissions.write.check(&path)? bec those method return anyhow::Error instead of std::io::Error(I don't know how to add path info to anyhow::Error and make path info print). I will very appreciate if you can help with this problem.

Permission errors coming from read.check() and write.check() already have paths printed out so there's no need to do anything there.

Would be great to see some updated to the unit tests that call assertThrows or assertRejects and verify that expected filepaths are included in the error message.

@bartlomieju sorry for slow reply. I am on the holiday. I will update the unit tests when i am back.

@dsherret do you have any suggestion as for the error messages?

@F3n67u please add some tests asserting error message contain paths so we can land this PR.

@bartlomieju I will back 2 days later. If you hurry, You can pick up and update the unit tests.

@dsherret do you have any suggestion as for the error messages?
@F3n67u please add some tests asserting error message contain paths so we can land this PR.

@bartlomieju I will back 2 days later. If you hurry, You can pick up and update the unit tests.

It's fine, enjoy your holiday :)

@F3n67u this is close to landing, there are two failures in tests on Windows.

@F3n67u this is close to landing, there are two failures in tests on Windows.

@bartlomieju Those two fail is fixed and test is included. ready to review again.

Tried it locally:

Deno 1.14.3
exit using ctrl+d or close()
> Deno.openSync("./non_existent")
Uncaught NotFound: No such file or directory (os error 2), open './non_existent'
    at Object.opSync (deno:core/01_core.js:138:12)
    at Object.openSync (deno:runtime/js/40_files.js:37:22)
    at <anonymous>:2:6
> Deno.renameSync("./non_existent", "./non_existent2");
Uncaught NotFound: No such file or directory (os error 2), rename './non_existent' -> './non_existent2'
    at Object.opSync (deno:core/01_core.js:138:12)
    at Object.renameSync (deno:runtime/js/30_fs.js:165:10)
    at <anonymous>:2:6

I think the messages could be improved further, but this is way better than current situation. Let's land it and improve messages in upcoming releases.