WIP: Tests! Coverage even!

[simonbuchan]

Uses:

• AVA for tests
• Istanbul NYC for coverage
  • With babel-plugin-istanbul to workaround NYC not understanding babel-compiled code

Still work in progress, but with an initial test working for the scary authenticateUser() I think it's worth starting to get feedback on if this is going the right direction.

Run npm test to just test, and npm run coverage to run with coverage and generate reports, currently:

• text summary on output
• lcov and HTML output in coverage

Getting NYC going correctly with babel is a bit sensitive right now, but Istanbul and NYC are moving fast right now, so I'm expecting some of the issues to disappear.

You can debug the tests if you install the HEAD version of AVA and use IDEA IntelliJ/WebStorm 2016.3 EAP: https://github.com/avajs/ava/blob/master/docs/recipes/debugging-with-webstorm.md

[simonbuchan]

Current coverage:

--------------------------|----------|----------|----------|----------|----------------|
File                      |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
--------------------------|----------|----------|----------|----------|----------------|
All files                 |    37.24 |       40 |    53.98 |    37.24 |                |
 AuthenticationDetails.js |    85.71 |       50 |       75 |    85.71 |             38 |
 AuthenticationHelper.js  |     0.98 |        0 |        0 |     0.98 |... 302,305,309 |
 CognitoAccessToken.js    |       40 |    33.33 |    66.67 |       40 |       42,43,45 |
 CognitoIdToken.js        |       40 |    33.33 |    66.67 |       40 |       42,43,45 |
 CognitoRefreshToken.js   |      100 |    33.33 |      100 |      100 |                |
 CognitoUser.js           |    41.97 |    44.97 |       60 |    41.97 |... 5,1153,1154 |
 CognitoUserAttribute.js  |       20 |       40 |    14.29 |       20 |... 60,61,68,75 |
 CognitoUserPool.js       |      100 |      100 |      100 |      100 |                |
 CognitoUserSession.js    |       70 |    42.86 |       80 |       70 |       28,63,65 |
 DateHelper.js            |    11.11 |        0 |        0 |    11.11 |... 46,49,52,54 |
--------------------------|----------|----------|----------|----------|----------------|

All coverage except CognitoUserPool and CognitoUser is semi-accidental and probably should be mocked out to be tested individually.

Files which are not loaded by a test, such as /enhance.js do not contribute, since babel-plugin-istanbul and nyc --all aren't mixy yet: istanbuljs/nyc#333 and istanbuljs/babel-plugin-istanbul#4

[itrestian]

looks good to me. Can I merge or should I hold off for a while?

[simonbuchan]

WIP: prefix in the title means "work in progress", so not yet.

It won't break anything, to be sure, since it doesn't touch the actual code, but there will be a fair bit more munging to make the tests less repeatitive / a bit easier to reason about, as I get further.

I want to get 100% function coverage at minimum, but I feel good about getting at least close to 100% branch.

This is fine to be a long-running PR, as it won't be touching the actual source, so no conflicts.

[itrestian]

Sounds good, I'll keep it open.

[simonbuchan]

Updated coverage, all work on authenticateUser():

% npm -s run coverage -- --serial --verbose

<snip old test results>

  69 tests passed [19:20:18]
  4 tests todo

-------------------------|----------|----------|----------|----------|----------------|
File                     |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
-------------------------|----------|----------|----------|----------|----------------|
All files                |     49.2 |    47.15 |     57.8 |     49.2 |                |
 AuthenticationHelper.js |     0.98 |        0 |        0 |     0.98 |... 302,305,309 |
 CognitoAccessToken.js   |       40 |    33.33 |    66.67 |       40 |       42,43,45 |
 CognitoIdToken.js       |       40 |    33.33 |    66.67 |       40 |       42,43,45 |
 CognitoRefreshToken.js  |      100 |    33.33 |      100 |      100 |                |
 CognitoUser.js          |    60.36 |    55.62 |    67.69 |    60.36 |... 5,1153,1154 |
 CognitoUserAttribute.js |       20 |       40 |    14.29 |       20 |... 60,61,68,75 |
 CognitoUserPool.js      |      100 |      100 |      100 |      100 |                |
 CognitoUserSession.js   |       70 |    42.86 |       80 |       70 |       28,63,65 |
 DateHelper.js           |    11.11 |        0 |        0 |    11.11 |... 46,49,52,54 |
-------------------------|----------|----------|----------|----------|----------------|

CognitoUser tests now use a mock AuthenticationDetails, so it's not listed any more: ideally everything except the tested types (CognitoUserPool and CognitoUser for now) would be mocked out so they can be tested separately and checked for coverage.

Just sendCustomChallengeAnswer(), sendMFACode() and the valid session checks left in CognitoUser.

The only real code left after that is AuthenticationHelper and DateHelper. The latter could be a pain without modifications, AFAIK you can't mock out built-in globals in another module in node (essentially the runtime adds them as module locals)

Looks like the valid session checks always call the callback as a node callback if they fail - it's possible I broke this with the ES2015 port. It should be safe to update the code to be correct, but I'd prefer to add those as failing but ignored tests then fix with a separate PR once this is done.

[simonbuchan]

Implemented fully mocking other modules, giving more accurate test coverage for tested files:

% npm -s run coverage -- --serial --verbose

  ✔ CognitoUser.authenticateUser › fails on initiateAuth => raises onFailure (1.2s)
  ✔ CognitoUser.authenticateUser › fails on respondToAuthChallenge => raises onFailure
  ✔ CognitoUser.authenticateUser › with new device state, fails on confirmDevice => raises onFailure
  ✔ CognitoUser.authenticateUser › hasOldDevice: false, hasCachedDevice: false, hasNewDevice: false => creates session
  ✔ CognitoUser.authenticateUser › hasOldDevice: false, hasCachedDevice: false, hasNewDevice: true => creates session
  ✔ CognitoUser.authenticateUser › hasOldDevice: false, hasCachedDevice: true, hasNewDevice: false => creates session
  ✔ CognitoUser.authenticateUser › hasOldDevice: false, hasCachedDevice: true, hasNewDevice: true => creates session
  ✔ CognitoUser.authenticateUser › hasOldDevice: true, hasCachedDevice: false, hasNewDevice: false => creates session
  ✔ CognitoUser.authenticateUser › hasOldDevice: true, hasCachedDevice: false, hasNewDevice: true => creates session
  ✔ CognitoUser.authenticateUser › hasOldDevice: true, hasCachedDevice: true, hasNewDevice: false => creates session
  ✔ CognitoUser.authenticateUser › hasOldDevice: true, hasCachedDevice: true, hasNewDevice: true => creates session
  ✔ CognitoUser.authenticateUser › hasNewDevice: true, userConfirmationNecessary: true => creates session
  ✔ CognitoUser.authenticateUser › CUSTOM_AUTH flow, CUSTOM_CHALLENGE challenge => raises customChallenge
  ✔ CognitoUser.authenticateUser › SMS_MFA challenge => raises mfaRequired
  ✔ CognitoUser.authenticateUser › DEVICE_SRP_AUTH challenge, fails on DEVICE_SRP_AUTH => raises onFailure
  ✔ CognitoUser.authenticateUser › DEVICE_SRP_AUTH challenge, fails on DEVICE_PASSWORD_VERIFIER fails => raises onFailure
  ✔ CognitoUser.authenticateUser › DEVICE_SRP_AUTH challenge, succeeds => creates session
  ✔ CognitoUser.authenticateUser › sendCustomChallengeAnswer() :: fails => raises onFailure
  ✔ CognitoUser.authenticateUser › sendCustomChallengeAnswer() :: CUSTOM_CHALLENGE challenge => raises customChallenge
  ✔ CognitoUser.authenticateUser › sendCustomChallengeAnswer() :: succeeds => creates session
  ✔ CognitoUser.authenticateUser › sendMFACode() :: fails on respondToAuthChallenge => raises onFailure
  ✔ CognitoUser.authenticateUser › sendMFACode() :: fails on confirmDevice => raises onFailure
  ✔ CognitoUser.authenticateUser › sendMFACode() :: hasOldDevice: false, hasNewDevice: false => creates session
  ✔ CognitoUser.authenticateUser › sendMFACode() :: hasOldDevice: false, hasNewDevice: true => creates session
  ✔ CognitoUser.authenticateUser › sendMFACode() :: hasOldDevice: true, hasNewDevice: false => creates session
  ✔ CognitoUser.authenticateUser › sendMFACode() :: hasOldDevice: true, hasNewDevice: true => creates session
  ✔ CognitoUser.authenticateUser › sendMFACode() :: hasNewDevice: true, userConfirmationNecessary: true => creates session
  ✔ CognitoUser › constructor() => throws "required" (1.2s)
  ✔ CognitoUser › constructor() => throws "required"
  ✔ CognitoUser › constructor(Username: null, Pool: null) => throws "required"
  ✔ CognitoUser › constructor(Username: null, Pool: "[mock UserPool]") => throws "required"
  ✔ CognitoUser › constructor(Username: "some-username", Pool: null) => throws "required"
  ✔ CognitoUser › constructor() :: valid => creates expected instance
  ✔ CognitoUser › setAuthenticationFlowType() => sets authentication flow type
  ✔ CognitoUser › confirmRegistration(forceAliasCreation: false) => fails
  ✔ CognitoUser › confirmRegistration(forceAliasCreation: true) => fails
  ✔ CognitoUser › confirmRegistration(forceAliasCreation: false) => succeeds
  ✔ CognitoUser › confirmRegistration(forceAliasCreation: true) => succeeds
  ✔ CognitoUser › changePassword() => fails
  ✔ CognitoUser › changePassword() => succeeds
  ✔ CognitoUser › enableMFA() => fails
  ✔ CognitoUser › enableMFA() => succeeds
  ✔ CognitoUser › disableMFA() => fails
  ✔ CognitoUser › disableMFA() => succeeds
  ✔ CognitoUser › deleteUser() => fails
  ✔ CognitoUser › deleteUser() => succeeds
  ✔ CognitoUser › updateAttributes() => fails
  ✔ CognitoUser › updateAttributes() => succeeds
  ✔ CognitoUser › getUserAttributes() => fails
  ✔ CognitoUser › getUserAttributes() => succeeds
  ✔ CognitoUser › deleteAttributes() => fails
  ✔ CognitoUser › deleteAttributes() => succeeds
  ✔ CognitoUser › resendConfirmationCode() => fails
  ✔ CognitoUser › resendConfirmationCode() => succeeds
  ✔ CognitoUser › forgotPassword() :: usingInputVerificationCode: false => fails
  ✔ CognitoUser › forgotPassword() :: usingInputVerificationCode: false => succeeds
  ✔ CognitoUser › forgotPassword() :: usingInputVerificationCode: true => fails
  ✔ CognitoUser › forgotPassword() :: usingInputVerificationCode: true => succeeds
  ✔ CognitoUser › confirmPassword() => fails
  ✔ CognitoUser › confirmPassword() => succeeds
  ✔ CognitoUser › getAttributeVerificationCode() => fails
  ✔ CognitoUser › getAttributeVerificationCode() => succeeds
  ✔ CognitoUser › verifyAttribute() => fails
  ✔ CognitoUser › verifyAttribute() => succeeds
  ✔ CognitoUser › getDevice() => fails
  ✔ CognitoUser › getDevice() => succeeds
  ✔ CognitoUserPool › constructor( null ) => throws with "required" (214ms)
  ✔ CognitoUserPool › constructor( {} ) => throws with "required"
  ✔ CognitoUserPool › constructor( {"UserPoolId":null,"ClientId":null} ) => throws with "required"
  ✔ CognitoUserPool › constructor( {"UserPoolId":"xx-nowhere1_SomeUserPool","ClientId":null} ) => throws with "required"
  ✔ CognitoUserPool › constructor( {"UserPoolId":null,"ClientId":"some-client-id"} ) => throws with "required"
  ✔ CognitoUserPool › constructor :: invalid UserPoolId => throws with "Invalid UserPoolId"
  ✔ CognitoUserPool › constructor => creates instance with expected values
  ✔ CognitoUserPool › constructor({ Paranoia }) => sets paranoia
  ✔ CognitoUserPool › setParanoia() => sets paranoia
  ✔ CognitoUserPool › signUp() :: fails => callback gets error
  ✔ CognitoUserPool › signUp() :: success => callback gets user and confirmed
  ✔ CognitoUserPool › getCurrentUser() :: no last user => returns null
  ✔ CognitoUserPool › getCurrentUser() :: with last user => returns user instance

  79 tests passed [20:35:19]

--------------------|----------|----------|----------|----------|----------------|
File                |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
--------------------|----------|----------|----------|----------|----------------|
All files           |    72.64 |    66.67 |    78.08 |    72.64 |                |
 CognitoUser.js     |    70.73 |    63.91 |    75.38 |    70.73 |... 5,1153,1154 |
 CognitoUserPool.js |      100 |      100 |      100 |      100 |                |
--------------------|----------|----------|----------|----------|----------------|

There are in fact a few other missed methods to test in CognitoUser, the device lists and session management: get/refreshSession(), signOut() etc... :(

I did find a few more probable bugs, check for comments around t.skip.xxx(...)

[bcoe]

@simonbuchan just landed some changes to nyc, istanbul-lib-instrument, and babel-plugin-istanbul, that adds support for --all. Would love the help testing:

npm cache clear; npm i nyc@next babel-plugin-istanbul@latest

I'm using configuration that looks something like this in my testing project:

cross-env NODE_ENV=test nyc --all --show-process-tree --silent mocha test/*.js

{"babel": {
    "presets": [
      "es2015"
    ],
    "env": {
      "test": {
        "plugins": [
          "istanbul"
        ]
      }
    }
  },
  "nyc": {
    "include": [
      "src/**/*.js"
    ],
    "exclude": [
      "node_modules"
    ],
    "require": [
      "babel-register"
    ],
    "sourceMap": false,
    "instrument": false
  }}

[simonbuchan]

New summary with --all:

--------------------------|----------|----------|----------|----------|----------------|
File                      |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
--------------------------|----------|----------|----------|----------|----------------|
All files                 |    52.45 |     48.8 |    50.44 |    52.45 |                |
 AuthenticationDetails.js |        0 |        0 |        0 |        0 |... 31,38,45,52 |
 AuthenticationHelper.js  |        0 |        0 |        0 |        0 |... 302,305,309 |
 CognitoAccessToken.js    |        0 |        0 |        0 |        0 | 28,35,42,43,45 |
 CognitoIdToken.js        |        0 |        0 |        0 |        0 | 28,35,42,43,45 |
 CognitoRefreshToken.js   |        0 |        0 |        0 |        0 |          26,33 |
 CognitoUser.js           |    70.73 |    63.91 |    75.38 |    70.73 |... 5,1153,1154 |
 CognitoUserAttribute.js  |        0 |        0 |        0 |        0 |... 60,61,68,75 |
 CognitoUserPool.js       |      100 |      100 |      100 |      100 |                |
 CognitoUserSession.js    |        0 |        0 |        0 |        0 |... 47,54,63,65 |
 DateHelper.js            |        0 |        0 |        0 |        0 |... 46,49,52,54 |
 index.js                 |      100 |      100 |      100 |      100 |                |
--------------------------|----------|----------|----------|----------|----------------|

[sbussard]

@simonbuchan it's great that you want to do this, but it's hard to get it "through the door" if it's such a big PR. Would you mind breaking it up into smaller chunks?

[simonbuchan]

To be honest? I hit a bit of a wall trying to figure out testing the stuff in the AuthenticationHelpers module, and we had already gotten our login UI working anyway.

If I were to pick this up again now from scratch, I would probably switch to jest since it has a much nicer runner, has better assert methods and output, and is far easier to get working properly, but the main thing is the code structure was a huge pain to work with without refactoring - but I wouldn't want to refactor that stuff without tests :(.

Now that you've reminded me, I might try taking another look at this now I've gotten more experience.

When you say "breaking it up", do you mean for code review purposes? The normal method I've seen and we've used internally has been to create a "landing branch" and create small PRs for review only merging to that, then merging that in without needing to review all at once.

[sbussard]

@simonbuchan I like the idea of a landing branch