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

Testing: Fix failure to run non-web translators' tests in debug build. #429

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

zoe-translates
Copy link

  • In the testTranslators tool included in the debug build, there is an incorrect script path preventing the type schema data from loading.
  • In cachedTypes.js, the callback passed to getSchema() is not called. (The call site is in translatorTester_viewer.js).

The overall effect is that non-web translators' test code did not run at all.

This is fixed by including the correct script path, and call the callback in getSchema().

- In the testTranslators tool included in the debug build, there is an
  incorrect script path preventing the type schema data from loading.
- In cachedTypes.js, the callback passed to `getSchema()` is not called.
  (The call site is in `translatorTester_viewer.js`).

The overall effect is that non-web translators' test code did not run at
all.

This is fixed by including the correct script path, and call the
callback in `getSchema()`.
@@ -178,7 +178,7 @@ Zotero.Connector_Types = new function() {
/**
* Passes schema to a callback
*/
this.getSchema = async function() {
return TypeSchema;
this.getSchema = async function(callback) {
Copy link
Author

Choose a reason for hiding this comment

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

This is supposed to make this work:

// other translators get passed right through, after we get schema and preferences
var me = this;
Zotero.Connector_Types.getSchema(function(schema) {
Zotero.Connector_Types.schema = schema;
Zotero.Connector_Types.init();
me._runTests(callback);
});

However I doubt whether this is the original intention. Why is it named "get"?

Copy link
Member

Choose a reason for hiding this comment

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

getSchema() was a callback-based function to get the schema, and it was changed to return a promise. So you just have this backwards — it's translatorTester_viewer.js that needs to be updated, not this.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks a lot for solving the mystery for me! That was what I suspected.

…sted.

This partially reverts 734faa3 and
restores the call signature of `getSchema()` (with the stale comment
about it removed). Instead, fix the caller by moving the callback to a
`then()` handler after it.
@@ -40,7 +40,7 @@ Zotero_TranslatorTester.prototype.runTests = function(callback) {
} else {
// other translators get passed right through, after we get schema and preferences
var me = this;
Zotero.Connector_Types.getSchema(function(schema) {
Zotero.Connector_Types.getSchema().then(function(schema) {
Copy link
Author

Choose a reason for hiding this comment

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

This is the "minimally invasive" change that I can think of. It did make the tests work again, however, I can definitely use some illumination about Zotero.Connector_Types and what it actually does.

Copy link
Author

Choose a reason for hiding this comment

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

I even tried hooking the access to the "schema" property on the Zotero.Connector_Types object so as to catch any code that tries to get the value of "schema", but I didn't find anything while running the tests. A search into the code seems to confirm that nothing else makes use of Zotero.Connector_Types.schema. So I don't know why this "schema" property is created and passed to the context of the test runs. What am I missing? Could @dstillman @AbeJellinek help? Thanks!

The file node_modules.js was deleted in
69627e7.
@adam3smith
Copy link

Thanks for working on this! dstillman -- If this will take a while to fix, could we disable the test runs for non-web translators? This is both confusing for new contributors and a hassle for everyone

@dstillman dstillman requested a review from adomasven June 19, 2023 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants