661615 - Testing the translator tutorial does not work with 1.0RC

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Will Bamberg [:wbamberg]]

Following the steps in the tutorial, the part in "Testing Your Module" does not work with the 1.0RC. When you execute "cfx --verbose test" you get something like this:

info: executing 'test-translator.test_german'
error: TEST FAILED: test-translator.test_german (exception)
error: An exception occurred.
Traceback (most recent call last):
  File "resource://jid1-fvcupmrdgevezw-translator-tests/test-translator.js", line 17, in
    test_languages(test, "Eidechse");
  File "resource://jid1-fvcupmrdgevezw-translator-tests/test-translator.js", line 13, in test_languages
    translator.translate(text, check_translation);
TypeError: translator.translate is not a function

The same code works fine with 1.0b5.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Irakli: any thoughts on what might be going on here?

Comment 2

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Works for me with current master:

(addon-sdk)➜  tutorial  (master) cfx test -v                                                                                                     ✱
Using binary at '/Applications/Firefox.app/Contents/MacOS/firefox-bin'.
Using profile at '/var/folders/-y/-yjxO9IXGGSRN-hSaip1Qk+++TM/-Tmp-/tmp6ECCP4.mozrunner'.
Running tests on Firefox 4.0/Gecko 2.0 ({ec8030f7-c20a-464f-9b0e-13a3a9e97384}) under Darwin/x86_64-gcc3.
info: executing 'test-tutorial.test_german'
info: pass: a == b == "Lizard"
info: executing 'test-tutorial.test_italian'
info: pass: a == b == "Lizard"
info: executing 'test-tutorial.test_finnish'
info: pass: a == b == "Lizard"
info: executing 'test-tutorial.test_error'
info: pass: a == b == "Text to translate must not be empty"

4 of 4 tests passed.


Also I think it would be better if in docs we will encourage simplified package structure compatible with node.js. In that case though {{package.json}}.main is required though so I'm not sure if it's worth it.

 /translator
      package.json
      README.md
      main.js
      translator.js
      /doc
          main.md
      /test
          test-translator.js

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

And btw google is shutting that service down so probably we should think of updating that tutorial in a future: http://code.google.com/apis/language/translate/overview.html

Comment 4

[David Mason [:dcm :dmason]]

Aw crap... this is what we were basing our video tutorial on....

Comment 5

[Myk Melez [:myk] [@mykmelez]]

I was talking to Matteo about this, as he also noticed the problem, and it looks like a regression from the loader changes.  Apparently, when the test does require("translator"), it is getting the translator package's main.js instead of its translator.js.

Comment 6

[Will Bamberg [:wbamberg]]

(In reply to comment #3)
> And btw google is shutting that service down so probably we should think of
> updating that tutorial in a future:
> http://code.google.com/apis/language/translate/overview.html

Thanks Irakli -> Bug 661857. Hopefully this needn't block 1.0.

Comment 7

[David Mason [:dcm :dmason]]

That page says it will run until December - so it definitely is not a blocker.

Comment 8

[Will Bamberg [:wbamberg]]

 
> Also I think it would be better if in docs we will encourage simplified
> package structure compatible with node.js. In that case though
> {{package.json}}.main is required though so I'm not sure if it's worth it.
> 
>  /translator
>       package.json
>       README.md
>       main.js
>       translator.js
>       /doc
>           main.md
>       /test
>           test-translator.js

That's interesting, and I did not know it. I thought we followed CommonJS package format, which keeps JS in "lib"? That's also what "cfx init" does of course. Should we have another bug to update all this?

Comment 9

[Brian Warner [:warner :bwarner]]

So, require("translator") is ambiguous: it could either mean "load the main entry point of the 'translator' package", or "search all packages for a module named 'translator'". In our linker's search rules, the entry-point form takes precedence. The synthetic 'main' property that packaging.py adds to the in-memory copy of package.json (used to tell the runtime loader where the addon starts) is being interpreted by the linker as meaning that this package has an entry point.

There's probably a better way for test-translator.js to point at the correct module. require("translator/translator") is the most explicit and should work fine, the only drawback being that you might someday rename the package (e.g. "translator-example" instead of just "translator"), and you'd need to update the internal references. So a relative name would be nicer, something that starts with "..", but here's where the existence of "sections" gets in the way. require("../lib/translator") is the most likely to work, but having "lib" in there seems kind of ugly.

Comment 10

[Will Bamberg [:wbamberg]]

Right: so this arises because the package and the module have the same name.

Is this something we want to resolve only in the translator tutorial, either by changing the "require" statement or by resolving the ambiguity? Or is there a tool change we want to make, to preserve the original behaviour?

If we want to resolve it in the translator tutorial only, then the other question: how should we encourage developers to refer to non-core modules in require() statements?

My initial reaction would be to resolve the package-module ambiguity by renaming the module to "translate-request" or something. But then should we still use the explicit require() syntax as well: 'require("translator/translate-request");'? I think we should, and we could maybe point at the "module-search" guide too...

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Brian: can you take this on, figure out which of the various options is the best one, and put together a patch for it?

Comment 12

[Brian Warner [:warner :bwarner]]

wbamberg reports that require("translator/translator") works, and the others don't. But I think it'll be safer and less confusing to teach devs to use distinct names, since otherwise this problem will come up again later. So I'm going to investigate 'mv translator.js something.js' and require("translator/something"), with some better value for "something".

Comment 13

[Brian Warner [:warner :bwarner]]

Attachment: change tutorial code to handle new linker behavior correctly

This fixes the failures that happen when you follow the instructions to
create and run unit tests for the sample 'translator' addon. The
require("translator") statement it used became ambiguous with the new linker
search behavior, and started fetching the "translator" package's entry point
(i.e. its main.js) instead of the module named translator.js .
    
The easiest (and best-practice-to-teach) fix seemed to be to rename
translator.js to translate.js, and to change the import to say
require("translator/translate"). I also fixed up the variable and test-file
names to match.
    
I also took the liberty of updating the output of 'cfx xpi' to match current
behavior (it no longer speaks of keypairs).

Comment 14

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to comment #5)
> I was talking to Matteo about this, as he also noticed the problem, and it
> looks like a regression from the loader changes.  Apparently, when the test
> does require("translator"), it is getting the translator package's main.js
> instead of its translator.js.

Right, that makes sense. I have not created `main.js` and that's why it worked for me.

Comment 15

[Will Bamberg [:wbamberg]]

Comment on attachment 537306 [details] [diff] [review]
change tutorial code to handle new linker behavior correctly

r+ with one change: the first H2 heading "Implementing "translator.js" should now be "Implementing "translate.js".

Thanks Brian!

Comment 16

[Brian Warner [:warner :bwarner]]

Landed, in https://github.com/mozilla/addon-sdk/commit/65c1cb9e9cb69ee8d305a0415d895ac44de77c4d

myk: you'll probably want to cherry-pick that one onto branch-1.0 .