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

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

Closed
wants to merge 10 commits into from
Closed

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

wants to merge 10 commits into from

Conversation

jehon
Copy link

@jehon jehon commented Mar 14, 2018

Fix bug #2955 :

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

@googlebot
Copy link

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
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
Copy link
Author

jehon commented Mar 14, 2018

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

Copy link
Member

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

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)

window.__karma__.loaded();
if (window.addEventListener) {
document.addEventListener("DOMContentLoaded",function(){
window.__karma__.loaded();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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
Copy link

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
Copy link
Author

jehon commented Mar 15, 2018

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

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

Successfully merging this pull request may close these issues.

4 participants