fs,test: throw rather than assert if condition is possible

[Trott]

Three commits:

fs: throw rather than assert on errant monkey-patching

It is possible to make `.close()` assert on an FSWatcher if the
`_handle` property is monkey-patched. Assertions should be reserved for
things that are believed to be logically impossible. Since we know end
users can cause the assertion, change it to a throw.

---

test: remove common.expectsInternal Assertion

Remove convenience function for internal assertions. It is only used
once.

---

test: add coverage for FSWatcher exception

Cover an previously uncovered exception possible in the internal start
function for FSWatcher.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[lpinca]

I prefer to keep it as is. Use of assertions to ensure that requirements are met is a common pattern used by many languages. Also the ERR_UNEXPECTED_INSTANCE custom error does not make sense in my opinion. It's an error that should not exist.

That said I won't block this if others are ok with removing assertions from the code base.

[Trott]

prefer to keep it as is. Use of assertions to ensure that requirements are met is a common pattern used by many languages.

I certainly am not excited about adding another error code--wish we had vastly fewer codes--so could leave the assert() part alone. This PR would then just add the test case and remove the expectInternalAssertion() from common.

[addaleax]

I’m feeling the same way as @lpinca – I’d prefer to keep these as assertions. Throwing a specific kind of error would make monkey-patching seem like a supported API feature. (Which it is not in this case.)

Assertions should be reserved for things that are believed to be logically impossible.

We do use assertions a lot in situations in which users can break them by monkey-patching or illegally accessing internals.

[Trott]

I’d prefer to keep these as assertions.

I could leave the assert() part alone. This PR would then just add the test case and remove the expectInternalAssertion() from common (which is undocumented, unused in other tests, and trivially wraps assert.throws()).

We do use assertions a lot in situations in which users can break them by monkey-patching or illegally accessing internals.

Would you be in favor, at least in theory, of semver-major hardening things like what's in this PR to make them more difficult or impossible to monkey-patch? Or would there be concern that will just make life harder for APM folks? (Maybe they already have better options?)

[nodejs-github-bot]

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

[Trott]

Closing in favor of #32057 which adds the test case and removes the common module convenience function, but leaves the internal asserts.