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 support for ADAL technology #42 #69

Closed
wants to merge 2 commits into from

Conversation

waldekmastykarz
Copy link
Contributor

Added support for ADAL technology solving issue #42.

@waldekmastykarz
Copy link
Contributor Author

It seems like the coverage dropped due to the optional appId prompt which is available only for ng-adal and which is included in the generators/app/index.js file outside of the specific subgenerators for which we have tests.
@andrewconnell does that mean that we need to add tests for ng-adal in the test/app.js file to test the appId prompt or is the current situation acceptable?
What's surprising is that if I run $ gulp test locally the coverage shows 100% rather than the 97.685% reported by coveralls.

@andrewconnell
Copy link
Contributor

Don’t trust what is reported by the gulp task… it’s a known issue that it isn’t the best on reporting usage. Coveralls is the authority here. You can run it locally and see what’s being missed by the test and what could be added to improve the test coverage. You can do this locally or view the online report. For instance this shows we do not have a test for this line: https://coveralls.io/builds/3823801/source?filename=generators%2Fcontent%2Findex.js#L506

@waldekmastykarz
Copy link
Contributor Author

All right. Let me see if I can add some test to keep the coverage intact.

@waldekmastykarz
Copy link
Contributor Author

@andrewconnell would it be okay to keep the coverage intact by changing the tech of for example the Taskpane generator to ng-adal at https://github.com/OfficeDev/generator-office/blob/master/test/app.js#L73? This would keep the base test the same and at the same time would allow us to test the ng-adal path which we are not testing currently and which is causing the coverage drop.

@waldekmastykarz
Copy link
Contributor Author

With regard to the differences between the results of $ gulp test vs. $ npm run-script test-travis: it seems like we might be having the issue described in SBoudrias/gulp-istanbul#37. Running $ gulp test shows a summary of 100% and 0 statements. If you run the travis test locally you get the same results as you get with coveralls on-line.

@andrewconnell
Copy link
Contributor

Sure… if the coverage doesn’t change then I don’t see what the problem would be with this

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.

None yet

3 participants