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

Integrate testing into the module #6

Closed
2 of 4 tasks
oliverfarrell opened this issue May 26, 2015 · 6 comments
Closed
2 of 4 tasks

Integrate testing into the module #6

oliverfarrell opened this issue May 26, 2015 · 6 comments

Comments

@oliverfarrell
Copy link
Owner

Moving this into it's own issue from #2 so it's easier to keep tabs on what needs doing in regards to tests.

  • Integrate Travis.ci
  • Integrate CodeClimate
  • Write lib/normalizeCli.js tests
  • Write perfbudget.js tests.
@oliverfarrell
Copy link
Owner Author

I'm just gonna open a discussion on this so that I'm clear what we're actually capable of testing considering the API requires a valid key to get anything valuable returned. I'm all new to this kind of testing so need somewhere to dump some thoughts.

  1. How would you go about writing these tests so that either an API isn't required to run the tests successfully or we create a config file that requires users to add their own API key before running tests? In this case we could use this file.
  2. How much of this can we actually test seen as most of the functionality is actually on the WPT API. As mentioned here we shouldn't really be testing the WPT API. Should be be checking that all the flags for on the CLI (but again that requires a valid API key and we'd need to test the values returned are the ones that match the flags).

Just a mind dump for now. Thoughts/input appreciated.

@stefanjudis
Copy link
Contributor

This is kind of a religious question, how to write unit tests. :bowtie:
But I'd love to share my opinion with you.

How would you go about writing these tests so that either an API isn't required to run the tests successfully or we create a config file that requires users to add their own API key before running tests? In this case we could use this file.

I simply wouldn't. Our tests should ( at least in my opionion ) test our functionality and not more.

E.g.

var depA = require( 'dep' );

var myModule = {
  doSomething : function() {
    var result = depA.something();

    if ( result ) {
      console.log( 'yeah' );
    } else {
      console.log( 'noooo' );
    }
  }
}

When you look at this tiny snippet, the functionality of myModule is not that complicated. The tests for the function doSomething have to cover two things. Log yeah when this dependency function call returns something truthy and log noooo if it's falsy.

That's it. So I'd go with mocking the dependency and control what this function will return to see if our code is behaving as it should.

For mocking dependencies two options come to my mind:

Either loading all the dependencies with a dependency injection system. Angular does it this way, and that's why it's so easy to test. You just inject mocked objects and can perfectly check how you code behaves in given cases. Needs some bigger refactoring.

Using something like https://www.npmjs.com/package/proxyquire. So changing the require( 'webpagetest' ) and mock the function call and see what our snippets does. :) Might work out of the box.

Anyways starting writing tests usually also leads to refactoring because testing a function most often shows problems that were not thought through.

I hope this explanation helps... And I can give it a shot if you like. :)

@oliverfarrell
Copy link
Owner Author

Buy that man a beer! 😄

Thanks for the explanation it's definitely what I needed – I'm gonna have a go at writing these this weekend and will let you know how I get on.

@stefanjudis
Copy link
Contributor

Buy that man a beer!

Haha. Maybe on a conf or something. :)
Glad my explanation helped.

I just recently tried proxyquire and it worked fine. So I'd give it also a try here. :bowtie:

@oliverfarrell
Copy link
Owner Author

Have every intention of doing this – been super busy of late. Will take a look ASAP.

@stefanjudis
Copy link
Contributor

No problem! :bowtie:

It's your project. I'm just here to help, if needed. ;)
And if I can support let me know. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants