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

feat(config): a 3rd option to run the tests by dynamically loading test scripts without using iframe or new window #2542

Merged
merged 5 commits into from
Feb 19, 2017

Conversation

chan1cyrus2
Copy link
Contributor

Implement a new option for user to run their test. The new option doesn't use iframe or a new window to run the tests. It combines both client.html and context.html files and then load the scripts dynamically when execute is received. The call flow remains the same as the other two options. This solution is used on our project that we need to support a lightweight browser which does not support both iframe and opening a new window. Since the scripts are loaded dynamically, the loading time is longer than the other two options.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. 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, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@chan1cyrus2
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@zzo
Copy link
Contributor

zzo commented Jan 20, 2017

@dignifiedquire what do you think?

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.

Thank you, this looks mostly good but some things are missing

  • tests: there should be at e2e test using this functionality in phantomjs and firefox
  • documentation: please add documentation and a description of this feature and when to use it. Also what its limitations are
  • there should be an option to override client_with_context.html in the same way the other templates can be overridden.

console.log Outdated
PhantomJS 2.1.1 (Linux 0.0.0)
PhantomJS 2.1.1 (Linux 0.0.0)
PhantomJS 2.1.1 (Linux 0.0.0)
PhantomJS 2.1.1 (Linux 0.0.0)
Copy link
Member

Choose a reason for hiding this comment

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

this seems like an unrelated change

Copy link
Member

Choose a reason for hiding this comment

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

Not even sure why this file exists in the first place tbh

@@ -0,0 +1,125 @@

<!DOCTYPE html>
Copy link
Member

Choose a reason for hiding this comment

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

.html.html?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my bad

@chan1cyrus2
Copy link
Contributor Author

Thanks!

I have added a e2e test case to cover the scenario (first time writing test case on cucumberjs, let me know if I do sth wrong) and updated the documentation on configuration.

I am not sure the part on overriding client_with_context.html in the same way the other templates can be overridden, can you elaborate more?

@chan1cyrus2
Copy link
Contributor Author

I have added a e2e test case to cover the scenario (first time writing test case on cucumberjs, let me know if I do sth wrong) and updated the documentation on configuration.

I am not sure the part on overriding client_with_context.html in the same way the other templates can be overridden, can you elaborate more?

@dignifiedquire
Copy link
Member

I am not sure the part on overriding client_with_context.html in the same way the other templates can be overridden, can you elaborate more?

there is an config option customContextFile, which is used here: https://github.com/karma-runner/karma/blob/master/lib/middleware/karma.js#L146 to allow the user to customise the html of the actual file being loaded in the browser. It would be good to have this option for the new template as well.

@chan1cyrus2
Copy link
Contributor Author

Thanks! I have added the option. Please review.

Copy link
Contributor Author

@chan1cyrus2 chan1cyrus2 left a comment

Choose a reason for hiding this comment

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

Add an option to override client_with_context.html

@dignifiedquire dignifiedquire merged commit aa42c41 into karma-runner:master Feb 19, 2017
chan1cyrus2 added a commit to chan1cyrus2/karma that referenced this pull request Mar 20, 2019
…g in parent mode without iframe

Back in karma-runner#2542, a third option to run tests without iframe is implemented, mostly for lightweight browser. This fix is to include file type so files like .css would able to load properly.
chan1cyrus2 added a commit to chan1cyrus2/karma that referenced this pull request Mar 22, 2019
…nt mode without iframe

Back in karma-runner#2542, a third option to run tests without iframe is implemented, mostly for lightweight browser. It only allows script element to be loaded dynamically. This fix includes file type like .css to be loaded properly.
johnjbarton pushed a commit that referenced this pull request Apr 12, 2019
…nt mode without iframe (#3289)

* fix(client): Includes attributes like type in script tags when running in parent mode without iframe

Back in #2542, a third option to run tests without iframe is implemented, mostly for lightweight browser. It only allows script element to be loaded dynamically. This fix includes file type like .css to be loaded properly.

Fixes #3289
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