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 configuration in ng2 blueprint updated, allowing unit testing on travis. #1738

Closed
wants to merge 2 commits into from

Conversation

andyskw
Copy link
Contributor

@andyskw andyskw commented Aug 18, 2016

The original karma configuration contained a custom launcher for travis (for more details, please check out karma.conf.js), but it was not used anywhere.

By extending the configuration, it is now automatically applied if travis is detected, and also enabled single run mode for the CI tool, otherwise karma wouldn't exit at the end of the tests.

I am not sure however, if adding an out-of-box travis support directly to the blueprint was the original intention behind the custom launcher.

…ated application to be unit tested on travis.

The original karma configuration contained a custom launcher for travis, but it was not used anywhere. By extending the configuration, it is now automatically applied if travis is detected, and also enabled single run mode for the CI tool.
@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.

@andyskw
Copy link
Contributor Author

andyskw commented Aug 18, 2016

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@filipesilva
Copy link
Contributor

filipesilva commented Aug 23, 2016

We currently allow doing single run via the ng test --watch=false. I understand how it would be helpful for the user to have that configuration directly in karma, but I think ultimately the config shouldn't "special-case" Travis since there are plenty of other CI's out there in general.

I appreciate you taking the time to do the PR, but that is my reasoning for not accepting it.

@andyskw
Copy link
Contributor Author

andyskw commented Aug 23, 2016

Thank you for the answer and the quick decision - you are absolutely right, I also don't think favoring one CI tool over the others would be a good direction for a general tool.

The PR was about extending the current travis-specific configuration of karma.conf.js, since there is already a custom launcher, called Chrome_travis_ci.

From the user's perspective, right now if you want to run your application against a CI tool, you will either have to edit your package.json or your karma.conf.js [you probably want to run your tests on your local machine as well sometimes, this is why adding the commands just to your CI tool's descriptor file (travis.yml for example) would not be enough probably], and there is nothing wrong with that.

But when the user generates the application, scans through the generated files, he will see, that there's already a Chrome_travis_ci launcher, which may lead to a false assumption (just as in my personal case), that this particular CI tool is supported. But this is not the case, which leads to frustration.

@filipesilva What do you think, would it make sense to remove the Chrome_travis_ci launcher from karma.conf.js, so that the split-brain situation would be solved completely? Travis users must edit files anyway, it would not make such a huge difference them to copy-paste 3 more lines, and it would make it clear, that there is no built-in support favoring one CI tool above any others.
If it does, I would be happy to give a PR for the change!

@filipesilva
Copy link
Contributor

You know, it completely slipped my mind that we already had the Chrome_travis_ci in customLaunchers. That changes things a bit.

I'd be interested in a PR that removes that from the Karma config, yes. If you are available to do it, I'd point you towards our .travis.yml file that already adds some specific code to make Protractor work in travis by 'faking' chrome: - if [[ "$TRAVIS_OS_NAME" == "linux" ]]; then export CHROME_BIN=chromium-browser; fi.

Maybe there's a good way to do something similar for Karma as well, and we could fully bypass custom Karma config for Travis.

@filipesilva
Copy link
Contributor

karma-runner/karma#1144 (comment) seems to have had success doing something similar.

@andyskw
Copy link
Contributor Author

andyskw commented Aug 23, 2016

Hmm, sure, removing the custom runner is not a big deal, I would love to give a PR for that. :)

So, as I understand it currently, in order to be able to run karma on any CI tools, there are two requirements that needs to be met:

  • we must run karma with singleRun: true
  • (for travis at least) we must be able to pass a --no-sandbox flag to chrome.

I can experiment with CHROME_BIN env var, maybe it can use the flag, and then the second requirement is met out-of-the-box.

The other one is more interesting here. Since the karma binary is not called directly, we can't pass --single-run to it. The test.ts could however inject the proper singleRun value to the options object, based on an environment variable (SINGLE_RUN?). It already does some preprocessing with the browsers array before creates the server.

This way, no travis specific code has to exist in the codebase, and the behavior could be controlled through environment variables (for example in .travis.yml).

@filipesilva What do you think about it? Isn't it too hacky?

@filipesilva
Copy link
Contributor

In ./tests/e2e/e2e_workflow.spec.js we are setting test args to run travis in single run mode, so no need to change that.

var testArgs = ['test', '--watch', 'false'];

The --no-sandbox arg might be a bit more complicated though... Not really sure how to do that one without changing karma config tbh.

@filipesilva
Copy link
Contributor

Maybe it's possible to change the CHROME_BIN=chromium-browser to CHROME_BIN="chromium-browser --no-sandbox" or something?

@andyskw
Copy link
Contributor Author

andyskw commented Aug 23, 2016

Okay, so here are my findings: ng init-ed repository with the new karma.conf.js and it's Travis builds.

Looks like CHROME_BIN does not work, but - echo "--no-sandbox" > ~/.config/chrome-flags.conf works flawlessly in .travis.yml. Also works with chromium for those, who prefer that one. You can check out the .travis.yml of the test project for all the settings I am using now.

@filipesilva
Copy link
Contributor

Oh that's nice! Your technique allows all Travis specific config to be ONLY on .travis.yml, which is just perfect.

For the CLI tests themselves I prefer chromium because it's already installed so it takes less CI time.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants