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

added debug argument to ng test task. #1799

Closed
wants to merge 14 commits into from

Conversation

abner
Copy link
Contributor

@abner abner commented Aug 22, 2016

added debug argument to ng test task. This argument when passed disable instrumentation and coverage, allowing to debug the source code when running tests. Fixes #1798


This change is Reviewable

abner added 2 commits August 22, 2016 06:41
…sources is not supported. If you are using the graceful-fs module, please update it to a more recent version.

�[2K�[1G�[?25h�[2K�[1G�[?25hYou have to be inside an angular-cli project in order to use the test command.. This argument when passed disable instrumentation and coverage, allowing to debug the source code when running tests
…le instrumentation and coverage, allowing to debug the source code when running tests
abner added 2 commits August 22, 2016 17:41
…instrumentation and coverage, allowing to debug the source code when running tests
@abner abner force-pushed the add-debug-argument-to-test branch from fda89ed to ad82f9a Compare August 22, 2016 20:43
@abner
Copy link
Contributor Author

abner commented Aug 23, 2016

@filipesilva I added this pull request to solve an issue related to not being able to see application code in debug karma session because of the instrumentation made to report the coverage. Basically i added a debug argument to ng test (ng test debug) which disable coverage and instrumentation parts and this way i could see my application code in chrome developer tool debug.
Could you review this and see if it would be merged.
Thanks in advance!

@filipesilva filipesilva added the needs: more info Reporter must clarify the issue label Aug 23, 2016
@filipesilva
Copy link
Contributor

Before I review, can I ask you to test in the latest master? We had a problem with sourcemaps that is fixed I believe, and would cause you to see instrumented code.

@TheLarkInn can you have a look at #1798 and tell me if this is the sourcemaps issue?

@TheLarkInn
Copy link
Member

No this is specifically related to instrumented code in testing mode.

@abner
Copy link
Contributor Author

abner commented Aug 24, 2016

I reviewed and it still occurs. As @TheLarkInn said it is related to instrumented code.

@abner
Copy link
Contributor Author

abner commented Sep 17, 2016

hi @TheLarkInn and @filipesilva !
I still need this feature in angular-cli. I updated the pull request with master.
Could you guys check if it would be merged?
Thanks in advance.

@filipesilva filipesilva added pr_action: review and removed needs: more info Reporter must clarify the issue labels Sep 22, 2016
@filipesilva
Copy link
Contributor

@abner I'm sorry it's taken so long to review this PR. I've bumped the original issue as something that needs to be finished for RC1 and we should get to it soon.

@abner
Copy link
Contributor Author

abner commented Sep 22, 2016

great @filipesilva . I just updated the branch.

@filipesilva
Copy link
Contributor

CI is showing a lint error:

/home/travis/build/angular/angular-cli/packages/angular-cli/models/webpack-build-test.js
  9:7  error  'debug' is already defined  no-redeclare

It's minor but I cannot merge it without a green CI.

@@ -5,6 +5,7 @@ import {CliConfig} from '../models/config';
const NgCliTestCommand = TestCommand.extend({
availableOptions: [
{ name: 'watch', type: Boolean, default: true, aliases: ['w'] },
{ name: 'debug', type: Boolean, default: false, aliases: ['d'] },
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this option being passed into the karma config? I don't think it's being passed in yet.

My suggestion is this: in karma.conf.js add in the debug property to the angularCli options:

    angularCli: {
      config: './angular-cli.json',
      environment: 'dev',
      debug: false
    },

Then you'll need to also change packages/angular-cli/tasks/test.ts to set this to true in the options variable that's being passed into karma.Server.

Make sure it works though, this is the kind of thing I don't know we can really do CI tests without doing really weird stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i actually implemented debug false as default so it doesn't need to be passed. i added an argument to the test command --debug (-d). I think we would add a npm script npm run test:debug which would call ng test --debug

Copy link
Contributor

Choose a reason for hiding this comment

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

I was tracing back the code and realize you are right. Since we just start karma with all the options that the command receives, the debug property will be there. My bad!

@@ -66,7 +67,7 @@ const getWebpackTestConfig = function (projectRoot, environment, appConfig) {
{ test: /\.(jpg|png)$/, loader: 'url-loader?limit=128000' },
{ test: /\.html$/, loader: 'raw-loader', exclude: [path.resolve(appRoot, appConfig.index)] }
],
postLoaders: [
postLoaders: debug ? [] : [
Copy link
Contributor

Choose a reason for hiding this comment

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

In the latest webpack update, the loader configs changed a bit and is now a single rules array.

You're probably going to have to add the sourcemap-istanbul-instrumenter-loader conditionally, but by itself, at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok @filipesilva . i will update this branch to the latest changes in master and fix the linting issue you mentioned.


const appRoot = path.resolve(projectRoot, appConfig.root);
var debug = typeof debug !== 'undefined' ? debug : false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Linting says this variable is being shadowed.

@filipesilva
Copy link
Contributor

@abner we were talking about this PR between the team, and thinking of a somewhat alternative solution.

How about having a --code-coverage (-cc) flag that adds the instrumentation, instead of having a --debug flag to disable it? I don't think users care about code coverage during their normal watch cycle, and it should make things faster as well.

What do you think?

@filipesilva filipesilva self-assigned this Oct 14, 2016
filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Oct 23, 2016
filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Oct 23, 2016
@filipesilva
Copy link
Contributor

Heya @abner, I ran with the idea of making code coverage optional, and also made linting optional in #2840.

filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Oct 23, 2016
filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Oct 23, 2016
filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Oct 23, 2016
filipesilva added a commit that referenced this pull request Oct 24, 2016
Heavily inspired by @abner's work on #1799.

Partially address #1980
Close #1799
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Source Code showed in Browser Developer Tools is showing instrumented source code.
4 participants