945748 - page-mod contentScriptWhen option is missing tests

Status: RESOLVED FIXED

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

Description

[Tomislav Jovanovic :zombie]

test each of the three options, in each of the situations:

1) new pages, loaded after the page-mod is creted
2) existing tabs, in various states when the page-mod is created
  a) just opened
  b) parsed (onReady)
  c) fully loaded

Comment 1

[Jeff Griffiths (:canuckistani) (:⚡︎)]

If you have tests, we'll take 'em.

Comment 2

[Tomislav Jovanovic :zombie]

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1334

it's actually 4 options (counting the default), so times the 4 tests makes 16 new asserts in total.

the common logic for these new tests is extracted into a helper function.

Comment 3

[Peter Retzer (:pretzer)]

Comment on attachment 8356717 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1334

In the future you should make sure to request review from an actual person, so it doesn't get lost. You can find suggested reviewers when you upload an attachment and click the 'suggested reviewers' link.

Comment 4

[Dave Townsend [:mossop]]

Comment on attachment 8356717 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1334

Can you review this Matteo?

Comment 5

[Tomislav Jovanovic :zombie]

Matteo: i (hope) addressed the comments from the PR, so r? again..

Comment 6

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8356717 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1334

Thanks a lot to addressed the previous review's comment! The final code is slightly different from what I had in mind, but is really good work! I r- only because the tests seems not passing; as soon as you fix it I will r+.
Check the comments on PR and let me know.

I would expect no further issues, so you can squash the commit in just one – as policy we merge on master only one commit per time, so usually we have more commits during the development and review, and then we squash them.
If you have any question, feel free to ping me!

Comment 7

[Tomislav Jovanovic :zombie]

Comment on attachment 8356717 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1334

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #6)
> only because the tests seems not passing; as soon as you fix it I will r+.
most likely because of recent changes in content/worker.js, see comments for details..

> Check the comments on PR and let me know.
i addressed all the comments and nits (except one about arrow functions).

> I would expect no further issues, so you can squash the commit in just one –
done

Comment 8

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/2d0749b4034cc7ed1b1e693b124913545fa2f3b8
bug 945748 - PageMod.contentScriptWhen tests

test that the PageMod is attached at the right time, depending on the
contentScriptWhen option value.

https://github.com/mozilla/addon-sdk/commit/d20dea064a81a52a70a4be46d0da74e08466c827
Merge pull request #1334 from zombie/945748-test-csWhen

fix bug 945748 - PageMod.contentScriptWhen tests @r=ZER0