Add test for validateSync to test ignore async functions but still run error hooks

[hasezoey]

Summary

This PR adds tests that test validateSync to execute sync functions and error hooks but ignore async function() and ignores returned Promises

fixes #8703

Blocked currently because validateSync does not seem to execute any hooks

[AbdelrahmanHafez]

If async validators don't work, I think we should throw an error instead of silently ignoring them. Similar to how we handle transform option, see #9176.
What do you think @hasezoey

[hasezoey]

If async validators don't work, I think we should throw an error instead of silently ignoring them. Similar to how we handle transform option, see #9176.

i dont know what the convention for this is here, though currently in #12371 doValidateSync silently ignores the async function (as stated in the documentation)

this PR here (#12372) focuses on hooks, which do not seem to be executed in validateSync currently

[AbdelrahmanHafez]

Interesting, I didn't know that.

So async validators would run with Document#validate(), but not with Document#validateSync(), nor implicitly with await user.save();. Is that correct? @hasezoey

[hasezoey]

Interesting, I didn't know that.

So async validators would run with Document#validate(), but not with Document#validateSync(), nor implicitly with await user.save();. Is that correct? @hasezoey

(already answered in #12371 (comment) with similar question)

from what i know, in both validate and save async validators are run (and promises "awaited") (from what i can tell, save calls validate)

[AbdelrahmanHafez]

Thanks @hasezoey
I just realized they are two different PRs, thought there was an issue with GitHub and my comments didn't go through, apparently I just duplicated them. 😄

[hasezoey]

rebased on latest master to resolve conflicts

[vkarpov15]

@hasezoey is this PR still valid? It's still in draft state and has been for a while, so I wanted to check if we can close this PR.

[hasezoey]

@hasezoey is this PR still valid? It's still in draft state and has been for a while, so I wanted to check if we can close this PR.

i have been waiting for feedback about the pr and the referenced issue, simply said this PR adds a test for a issue, but the functionality is not quite what this test is meant to test and there has not been feedback about what to do (is this wanted behavior or should be changed)

if i remember correctly, the current issue is about that validateSync states that it does not call any async hooks but should still call (sync) error handling hooks, which it seemingly does not

i am not sure what the exact problem is / was because the logs have expired and i am currently not on my dev machine

[vkarpov15]

Ok, I'll take a closer look tomorrow

[hasezoey]

rebased onto latest master to resolve conflicts

current issue is that both tests fail because none of the 3 validate hooks are called, and i dont know if this is expected or unexpected behavior (meaning if the code should be changed to run validate hooks, or the tests to be changed to test that none are executed)

[vkarpov15]

So I think the confusion is due to the fact that I mixed up async hooks and async validators when I wrote up #8703. Right now it is expected behavior that validateSync() skips all middleware, and I'm not convinced it is worthwhile to make validateSync() execute sync middleware.

Right now we only have one function that supports sync middleware, init(), and that code feels a bit hacky. There's also the concern about validateSync() firing pre('validate') and post('validate') hooks as opposed to pre('validateSync') and post('validateSync'), which may be a better API.

Thanks for the work you put in on this PR @hasezoey , and I'm very sorry it took me so long to take a more thorough look. However, I'm going to close this PR and #8703 because I'm not convinced that executing validation hooks on validateSync() is worth doing ATM.