These are chat archives for gitevents/core

13th
Jan 2016
Michael Barrett
@mike182uk
Jan 13 2016 12:36

Howdy

So im undertaking the task of adding tests to core to try and stabilise things. I've started with lib/events.js. Looking at the existing tests, there is only 1 that is not skipped and when this is run it fails anyway. I went down the route of trying to fix it, but it seems as soon as i resolved one issue another one cropped up, so i've tried to tackle it with a fresh set of tests.

The problem i have found is there is just too much code inside of lib/events.js to easily and effectively test. Tests require a crazy amount of setup code and its just unmanageable.

The approach i have decided to take is to split up the module into smaller modules, and add tests to the smaller modules. This will gradually build up to covering the module as whole once we have extracted enough. I've currently made a start on this here: gitevents/core#152.

In this PR i have extracted some of the github logic and the logic for adding a talk + creating / updating an event. I am cautious about committing a massive PR containing tons of changes to lib/events.js so i want to try stage the changes - extract + test, test functionally, then merge into master.

I would like to look at getting this merged in (if you guys are cool with the approach) but i need to test it from a functional perspective. I am not 100% sure on the workflow / how the system works and could really do with somebody guiding me through some functional testing, to make sure that everything is still happening that should be (well just the events.js part of the system)

This message was deleted
Michael Barrett
@mike182uk
Jan 13 2016 12:48
On the PR ignore the obvious code style issues for now, once the functional tests confirm everything is ok i'll resolve them.
Patrick Heneise
@PatrickHeneise
Jan 13 2016 12:55
Hey
are we really there that 265 lines of code is "too long" for a library?
addTalk and createOrUpdateEvent need to be tested separately, but all the stuff belongs together, so I don't see a reason to separate it into various files tbh
Patrick Heneise
@PatrickHeneise
Jan 13 2016 13:02
wrapping a helper library (github) in another helper library (githubHelper) seems incredibly overhelping to me?
Niko Nevala
@nnevala
Jan 13 2016 13:03
I don't think the number of lines in the library matters, but rather the length of methods and lever of abstraction inside those methods
so if you look at, for example, testing the createOrUpdateEvent method, it's nigh impossible because it's 1) very long and 2) very tightly coupled with a 3rd party code
i.e. createOrUpdateEvent sounds like a very high level method but it's actually doing long level stuff
so I think wrapping the github library makes sense, as then we're at least talking to an interface we control, and we don't have to test everything at integration level
Patrick Heneise
@PatrickHeneise
Jan 13 2016 13:13
Ok, but shouldn't we be able to trust a tested npm module?
you wouldn't wrap 'async' in a 'asyncHelper'
the abstraction looks nice of course, since you can skip 2 lines or code (repo and user in the arguments)
Niko Nevala
@nnevala
Jan 13 2016 13:14
it's not really about trust, but the level of abstraction and testatbility
I wouldn't wrap async in asyncHelper, but I wouldn't (probably, maybe) call async directly from a high level method either
Patrick Heneise
@PatrickHeneise
Jan 13 2016 13:15
hmm
I see github as the implementation of the low-level stuff, the helper abstraction doesn't do anything else than wrapping the github call into a promise as I see it, that's the same functionality as before. Since you can pass github around as argument, you could replace it with a mock instead of intercepting with nock
Niko Nevala
@nnevala
Jan 13 2016 13:18
it also encapsulates the configuration
Patrick Heneise
@PatrickHeneise
Jan 13 2016 13:18
yes, but that's 2 lines of code for each call you safe - that's not an argument for a whole new library
Niko Nevala
@nnevala
Jan 13 2016 13:18
I'm not that concerned about the number of lines
Patrick Heneise
@PatrickHeneise
Jan 13 2016 13:19
there's no further processing in the helper, it just passes the arguments back and forth .. if you want to have promises, you could use promisifyAll
to my understanding right now: github is a low-level library that is tested and can be trusted. It's not 100% nice to call as you need to pass user and repo all the time, but it doesn't need to be wrapped in another helper IMHO
it's complicating things by oversimplification ... but I'd like to get another opinion on this
Niko Nevala
@nnevala
Jan 13 2016 13:21
fair enough. I'm a fan of the approach in that PR, and I would probably take it even further to get rid of the buffer and json handling in that method.
Patrick Heneise
@PatrickHeneise
Jan 13 2016 13:21
cc @iancrowther
Niko Nevala
@nnevala
Jan 13 2016 13:22
so, how would you approach testing that method then?
like Mike mentioned, once the internals are exposed testing it requires a lot of setup, and ultimately ends up testing things that are not really in the domain of this application
or alternatively testing everything at an integration level
Patrick Heneise
@PatrickHeneise
Jan 13 2016 13:27
createOrUpdateEvent and addTalk are private functions that should and can only be called from the main part ... an idea to test them could be to add a condition in the export function ... if (process.env.NODE_ENV === 'test') { export all functions } else { export main function }
Niko Nevala
@nnevala
Jan 13 2016 13:30
hmm, so not test those functions themselves at all? and in test mode return test versions to gitevents.js?
Patrick Heneise
@PatrickHeneise
Jan 13 2016 13:30
ian suggested splitting createOrUpdate into 2 functions, create and update, which makes sense. There'll be also an updateTalk function, that makes 4 private functions which call low-level github stuff and one main function dealing with the conditions. On an abstraction layer I think that's quite sufficient?
no, of course test the functions separately
Ian Crowther
@iancrowther
Jan 13 2016 13:31
reading...
Patrick Heneise
@PatrickHeneise
Jan 13 2016 13:32
it's just an idea I've never tried before, instead of just doing export.module = exports = function() we could export conditionally, if the module runs in dev or production, export the function, if it runs in testing, export an object with all the private functions
Ian Crowther
@iancrowther
Jan 13 2016 13:33
ok im bowling in...
the github mod does not need testing explicitly, we trust that is sane..
if its not, we contrib back to that mod and make it sane
so thats a green black box!
lol ^^
anyhow.
we do need to test the methods on the controller
eg create
update
even private methods need unit tests
can use mock’s etc
i just want to know that our private stuff is robust
make sense?
Patrick Heneise
@PatrickHeneise
Jan 13 2016 13:35
if (process.env.NODE_ENV === 'test') {
  module.exports = {
    addTalk: addTalk,
    addEvent: addEvent,
    ...
  }
} else {
  module.exports = exports = function events(payload) { ... }
}
Niko Nevala
@nnevala
Jan 13 2016 13:35
I don't think there was any disagreement on any of the above
Ian Crowther
@iancrowther
Jan 13 2016 13:35
ok cool
Patrick Heneise
@PatrickHeneise
Jan 13 2016 13:36
does this make sense?
Niko Nevala
@nnevala
Jan 13 2016 13:36
more about the specific approach to the testing
Ian Crowther
@iancrowther
Jan 13 2016 13:36
semantics, but dont like the xxxFile format
is redundant
making use of process.env.NODE_ENV node env makes sense
its what its there for!
Patrick Heneise
@PatrickHeneise
Jan 13 2016 13:38
re naming of functions, that's not really consistent and my fault ... addTalk and createOrUpdateEvent is crap
Ian Crowther
@iancrowther
Jan 13 2016 13:38
fine, we can evolve that out
no stress
Patrick Heneise
@PatrickHeneise
Jan 13 2016 13:38
but they do create files/entities on GitHub so it should be something like createEvent/createTalk/updateEvent/updateTalk
Ian Crowther
@iancrowther
Jan 13 2016 13:38
what am i missing test wise?
the fact that writing unit tests requires a lot of setup?
Patrick Heneise
@PatrickHeneise
Jan 13 2016 13:39
if the above solves our private function testing problem, we can remove the 'abstraction' of the github module and test these functions directly, by passing a mock-github to them in the tests
or use nock to intercept, that kind of works well
Ian Crowther
@iancrowther
Jan 13 2016 13:41
ok
Patrick Heneise
@PatrickHeneise
Jan 13 2016 13:41
@nnevala d'accord?
or veto?
Ian Crowther
@iancrowther
Jan 13 2016 13:43
gents? @nnevala @mike182uk
Niko Nevala
@nnevala
Jan 13 2016 13:44
hehe. I don't think I can veto anything here, but I disagree :) anyway, it's your project so it's your call
Patrick Heneise
@PatrickHeneise
Jan 13 2016 13:44
it's not my project
this is a collaborative effort so it's a democratic decision
Ian Crowther
@iancrowther
Jan 13 2016 13:45
@nnevala its an oss project
this is an open debate with equal share
were having a healthy debate to agree on a pattern that we can all follow
so you disagree..
can you expand and help me understand?
your an exp dev, your opinion matters!
Niko Nevala
@nnevala
Jan 13 2016 13:50
well, I basically laid out my arguments earlier. I dislike mocking third party code (it's very fragile, time consuming and ends up testing the wrong thing), and I dislike mixing abstraction levels (high level function doing low level stuff as it makes the code harder to read). if you get the abstraction levels right you kind of solve both problems in one go... your code becomes easier to comprehend and easier to test as you can push the components that cross system boundaries to the edges and test them separately from your application's domain logic
Ian Crowther
@iancrowther
Jan 13 2016 13:52
ok
Niko Nevala
@nnevala
Jan 13 2016 13:52
that's obviously an opinion coming from an enterprisey context
Ian Crowther
@iancrowther
Jan 13 2016 13:53
and experience
@PatrickHeneise thoughts?
Niko Nevala
@nnevala
Jan 13 2016 13:53
and lot of the OSS stuff I've seen doesn't really play by the same book. the preference is usually on practicality and speed of delivery over other values, and usually you have more consistency in maintainers etc which means it's ok to expect more knowledge of the codebase in order to be able to work with it
so, ultimately it's kind of a value and policy question :)
in this specific instance, however, we are talking about testing. maybe it's not worth pursuing testing the core too much?
testing as in automated code testing
Patrick Heneise
@PatrickHeneise
Jan 13 2016 13:56
createEvent
- github.get content
  - github.create content
  OR
  - github.update content

createEvent
- helper.get content
  - github.get content
    - helper.create content
      - github.create content
at some point you need to call low-level code from high-level functions, by wrapping it in a helper you do exactly the same (there's no logic in the helper) which just adds more complexity
Niko Nevala
@nnevala
Jan 13 2016 13:56
anyway, somehow I wound up taking a stand here. I'd be interested in hearing more from @mike182uk directly, seeing how it's his implementation
Patrick Heneise
@PatrickHeneise
Jan 13 2016 13:57
if you'd do some conditional checking and further processing in the helper, it would be legit, but not as it is right now
Niko Nevala
@nnevala
Jan 13 2016 13:57
you wrap low level code in order to encapsulate details that are not relevant in higher levels
Patrick Heneise
@PatrickHeneise
Jan 13 2016 13:58
"in this specific instance, however, we are talking about testing. maybe it's not worth pursuing testing the core too much?" - given it's the core it should be the most stable and well tested part of the whole system
Niko Nevala
@nnevala
Jan 13 2016 13:59
yep, meant automated testing in specific. maybe it's sufficient to test it manually?
Patrick Heneise
@PatrickHeneise
Jan 13 2016 13:59
nope, really not ... manual testing of that thing is a bitch
that's what I've been doing most of the time and could have written tests 3 times in the time I've wasted :/
note: lesson learned
Michael Barrett
@mike182uk
Jan 13 2016 14:06

So yeah, reading back through, i agree with what niko is saying.

I split things up to make them easier to test and reason about.

Michael Barrett
@mike182uk
Jan 13 2016 14:11
As for using node.env to expose the private methods, i dont think this is a good idea. The methods are supposed to be private for a reason.
Ian Crowther
@iancrowther
Jan 13 2016 14:13
back in 5
Patrick Heneise
@PatrickHeneise
Jan 13 2016 14:13
the methods remain private. Just not for the testing environment
Patrick Heneise
@PatrickHeneise
Jan 13 2016 14:21
btw. by putting the functions in separate files you're exposing stuff that should be private ;)
create-update-event.js for example is now a public function which it really shouldn't be, as it's only called by the event module. If from anywhere else, it will break
my vote goes for splitting up createOrUpdate into create and update, rename functions to be consistent, but keeping everything in events.js, exposing the private functions only and exclusively in env.TEST and therefore making the github helper obsolete.
@nnevala and @mike182uk are pro githubhelper and splitting things up further. @iancrowther where do you stand?
is that summary so far correct?
Ian Crowther
@iancrowther
Jan 13 2016 14:32
hey
im not sure about exposing private methods via test env
never seen that before
its either private or its not
and im not too fussed about keeping things private or not, it js
mike is trying to add granularity for tdd, i get that
but is a bit much
createEvent
- helper.get content
  - github.get content
    - helper.create content
      - github.create content
this seems like an abstraction too far
Ian Crowther
@iancrowther
Jan 13 2016 14:37
i dont get why
createEvent
- github.get content
  - github.create content
  OR
  - github.update content
is adding so much completixy to the testing?
can we summarise what we have agreed upon and what needs resolving?
@mike182uk @PatrickHeneise
Ian Crowther
@iancrowther
Jan 13 2016 14:49
gents? have we hit a crossroads here?
Michael Barrett
@mike182uk
Jan 13 2016 14:51
I don't think anything has been agreed
my vote goes for splitting up createOrUpdate into create and update, rename functions to be consistent, but keeping everything in events.js, exposing the private functions only and exclusively in env.TEST and therefore making the github helper obsolete.
@nnevala and @mike182uk are pro githubhelper and splitting things up further. @iancrowther where do you stand?
Ian Crowther
@iancrowther
Jan 13 2016 14:52
ok
@mike182uk can you explain to me whats wrong with patricks general approach
eg why it tough to test
im not seeing it, as im not coding it atm..
Michael Barrett
@mike182uk
Jan 13 2016 14:53
For the reason i stated to begin with
The problem i have found is there is just too much code inside of lib/events.js to easily and effectively test. Tests require a crazy amount of setup code and its just unmanageable.
Patrick Heneise
@PatrickHeneise
Jan 13 2016 14:57
you could take out githubapi in events, pass github on as an argument in the main public function and for tests, just pass a mock as you already do?
no overhelping/overabstracting required
Michael Barrett
@mike182uk
Jan 13 2016 15:08
Im not a fan of the nock method - this requires knowing about how the github module works and what calls it making to the API and what its returning etc.
Michael Barrett
@mike182uk
Jan 13 2016 15:15
Using our helper to make these requests means we do not have to care about how the github module works at this level of the tests. We only need to care that the appropriate method on our helper is called. The helper has then got its own set of tests external from this one that verifies its interacting with the github module as required.
Patrick Heneise
@PatrickHeneise
Jan 13 2016 15:21
you're testing a lib that does nothing else than using (not even processing or modifying) another lib which is already tested - if you don't like nock you can use sinon or mock the same way you're using it on the helper and other tests already. Reducing a library called 'events' to 3 lines of code which are just function calls to other files which are using helpers to use npm modules is just going too far for me
there's really no intelligence or processing in the helper and all you test is if the helper works
modify the function call module.exports = function events(payload, github) ..., pass on the githubMock and you're good to go, without all the extra. I'm not going to proceed the discussion, vote on it and proceed as voted
looking at the PR you're introducing too much complexity instead of taking it away IMO
@iancrowther @mike182uk @nnevala thumbs up or down on the helper and splitting it all up making 'events.js' obsolete as it just calls 2 functions in other files.
Niko Nevala
@nnevala
Jan 13 2016 15:33
well, I'm with mike on this one
Michael Barrett
@mike182uk
Jan 13 2016 15:52

The helper deals with resolving / rejecting the promise based on the result of the callback from the Github module as well as making sure the correct config values are passed to the Github library.

Anyways, i don't think what i have introduced is adding more complexity, so it would be a thumbs up for me.

But if it is felt that what i have done is over simplification/over abstracting/adding too much complexity then i do not think this is right for the project long term and different avenues should be explored.

Patrick Heneise
@PatrickHeneise
Jan 13 2016 15:58
to get a promise-based result of the callback, you can just use http://bluebirdjs.com/docs/api/promise.promisifyall.html
Ian Crowther
@iancrowther
Jan 13 2016 16:18
the goal - to see a set of robust tests
if we need the abstraction to achieve that, then ok
i get both arguments
the abstraction is not doing a great deal for the additions
but, it does allow us to test the configs and promise callbacks
so how about this...
@mike182uk keep going in the branch
lets get the results...
get to a solid state, a state we are confident in
right now, i dont know if the core will grow or shrink etc
what i do know, is the tests will allow us to do that safely
especially if this is in production
which it is!
Patrick Heneise
@PatrickHeneise
Jan 13 2016 16:24
alright, I'm not going to argue (I do have a job as well) ... but nothing gets merged into master before its stable. Master is already deployed to docker and I have a gitevents instance running for BarcelonaJS already, so until everything is ready, finished, clean and tested, it should remain in a branch
Ian Crowther
@iancrowther
Jan 13 2016 16:24
im onboard with that
Patrick Heneise
@PatrickHeneise
Jan 13 2016 16:25
focus on simplicity please. The repo is open source and everyone should be able to understand what's going on there, to attract further contributors
Ian Crowther
@iancrowther
Jan 13 2016 16:25
agreed.. this has to be approachable
readme’s MUST refelct this
have to tell people hwo to contribute
there are two conflicting views here..
both have valid pro’s and con’s so its toughie...
lets just be stable..
lets the tests be the docs..
ok, lets put to bed now as we have to crack on...