Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Translator tester: Only test URL-specified translators when ready. #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zoe-translates
Copy link

Wait for translatorTestViewsToRun to be populated, and then call the handler for the URL-specified translators' tests. If the translatorTestViewsToRun object is not ready, the handler (runURLSpecifiedTranslators) will silently skip the tests it is supposed to run.

Fixes #20.

Wait for `translatorTestViewsToRun` to be populated, and then call the
handler for the URL-specified translators' tests. If the
`translatorTestViewsToRun` object is not ready, the handler
(`runURLSpecifiedTranslators`) will silently skip the tests it is
supposed to run.

Fixes zotero#20.
@@ -474,7 +475,7 @@ async function init() {
// get translators, with code for unsupported translators
if(!viewerMode) {
let translators = await Zotero.Translators.getAllForType(translatorType, true);
haveTranslators(translators, translatorType);
whenReadyToRun.push(haveTranslators(translators, translatorType));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works, but it isn't very good IMO to depend on non-obvious promise behavior.

When you call a function that returns a promise, the code in that function starts executing immediately. It only pauses on the first await. In this case, it awaits pretty early on and we don't await anything in this function between here and the new Promise.all(), so everything works as expected, but that's pretty fragile. If haveTranslators() should only run once translatorTestViewsToRun is ready, we shouldn't call it at all until that's the case.

So instead we could do something like

whenReadyToRun.push({ translators, translatorType });

and below:

for (let { translators, translatorType } of whenReadyToRun) {
	await haveTranslators(translators, translatorType);
}

(That change also makes the different translator types run sequentially instead of in parallel, which is probably more what we want here.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about reverting this change, and simply do (pseudo-javascript)

return or await haveTranslators(translators, translatorType);

on this line? This too works, and it's shorter.

I think here, it's not that haveTranslators() has to run after translatorTestViewsToRun is ready. Actually haveTranslators() is the function that makes translatorTestViewsToRun ready by populating it.

The problems in init(), on the line 428 with await Promise.all(... big arrow function), is that the arrow function creates runaway promises. In each loop in .map() the async arrow function calls the promise-returning haveTranslators(), forgets about it, and immediately returns (a promise that resolves to) undefined. So the await Promise.all() doesn't wait for the promises that do the actual work, only the promises that initiate the work (results of calling the arrow function). That's why this await Promise.all(...) resumes prematurely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants