fix: karma.loaded() started too early (#2955)

[jehon]

Fix bug #2955 :

• start karma.loaded() when modules are loaded
• add a test for module based tests

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

[jehon]

I signed it!

[googlebot]

CLAs look good, thanks!

[dignifiedquire]

This needs to be changed such that it always works in all supported browsers, as we will otherwise break compatibility for those (this includes IE 7)

[johnjbarton]

I think we should use "Load" here rather than "DOMContentLoaded". Any content in any tag or even could be needed by a test.

[jehon]

Wouldn't this change the actual behavior, where tests are run without waiting for all content to be loaded?

[dignifiedquire]

it might be better to make this an option rather than changing the behavior for everyone where it worked well so far

[jehon]

What could be an option? the type of event (loaded vs DomContentLoaded) or the whole new stuff?

[johnjbarton]

In existing code, the __karma__.loaded() is called after all the script tags are added to the document, at then end of the document. Adding an event listener will cause an extra delay, the .loaded() function will run in a different event-loop turn, and the stylesheet/css content will be loaded. Any of these three could cause some marginal tests to fail, but such tests are working outside of the expectations of the system. Tests that rely on time between outer-code load and test start, on synchrony with the outer-code load, or on the non-existence of resources lie outside of what karma-runner was designed to support IMO.

So the real question is whether this affects any of the popular test frameworks, which could have dependency on how loaded() is called. I guess the most likely issue would be the extra event-loop turn. I can't see how loaded vs domcontentloaded could make a difference except for cases with tight timeouts.

[jehon]

I agree with @johnjbarton analysis, and my conclusion of this analysis is that waiting for DomContentLoaded or for Loaded event is not a breaking change.

What is not sure for me is why you would want to use this pull request to wait longer, for "loaded" event? I think that you are right, it is a small delay more, out of the boundaries of the expected, but why do you want that more than just what is necessary for my bug fix?

[johnjbarton]

Because the issue that you have -- failure of tests because an asynchronously loaded resource is missing -- is the same problem we have for other resources. For many, many tests that have nothing other than JS, loaded vs documentcontentloaded should make no difference. Only if we have resources we want will it make a difference, and that difference will be positive.

[jehon]

I propose to make this PR pass with the minimum change in behavior, and to open another ticket for the behavior you describe. If this PR pass, it will be very easy for you to make this new PR.

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

[jehon]

I have to stop this pull request because I used a wrong email in one of my commits. I will create a new one, referencing this one.
Sorry for inconvenience