cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Braden Shepherdson <bra...@chromium.org>
Subject Re: Refactoring the CLI tests
Date Fri, 08 Nov 2013 19:17:41 GMT
On Fri, Nov 8, 2013 at 1:48 PM, Lorin Beer <lorin.beer.dev@gmail.com> wrote:

> >What we're aiming for here is that we mock as little as is practical,
>
> I think that kind of defeats the idea of unit tests in the first place, I
> like the clean delineation between unit tests and integration tests, and it
> sounds like both need to be improved. Mocking things at the boundary of the
> CLI is in line with this.
>
>
Who said I was writing a unit test? We're writing a separate directory of
integration tests, and we're hoping to keep them fast enough to call every
time.


> >We'll do it first, and then solve the speed problems later if any arise.
>
> agreed, and we can mitigate potential problems by keeping unit tests small
> and within the boundaries of the CLI.
>
> >My main frustration with the current state of the tests is how fragile
> some
> of them are with respect to even minor changes in the CLI. As I see it,
> this is closely related to mocking the file system calls.
>
> Yeah, mocking file system calls is a little goofy, especially considering
> the scope of the CLI as a project. We can reasonably expect  a user to have
> an HD, no?
>
> My philosophy on testing is not to waste time optimizing tests, it's to
> keep the tests small and lightweight. Each test deals with the smallest
> unit of code or functionality possible. That's what makes them fast.
> Integration tests deal with functionality that encompasses
> inter-process/inter-project/network communication, and where latency is
> expected.
>


Many of our existing "unit" tests, at least of the main workflows like
platform.add and plugin.add, are more like audits of the implementation
than real unit tests. They're not testing the behavior of the functions
("should install the plugin") but the way the function works ("should check
this file exists, then copy this file, then exec this command"). That's
fragile and lame, and doesn't test as much as we'd like.

The tests for things like the lazy-loader or hooker are much better,
testing things like "I'm going to fire the hooks for some event and it had
better call my registered hooks".

I hope that eventually these "integration" tests are (a) fast enough to run
every time, and (b) a replacement for the existing `platform` and `plugin`
tests.

Braden


>
> On Thu, Nov 7, 2013 at 4:32 PM, Mark Koudritsky <kamrik@google.com> wrote:
>
> > My main frustration with the current state of the tests is how fragile
> some
> > of them are with respect to even minor changes in the CLI. As I see it,
> > this is closely related to mocking the file system calls. CLI is all
> about
> > massaging the files around, most of the state we care about lives on the
> > file system. So once we mock the fs calls and can't check for existence
> and
> > contents of files, we don't have much to check for in the tests, so we
> > start checking implementation details like specific function calls and
> then
> > it gets fragile. As far as I understand, the kind of relationship that
> CLI
> > has with the file system is not so common with other Node.js based
> projects
> > and the distinction between unit and integration tests is somewhat
> blurred
> > here.
> >
> > Anyway, as Braden said, we'll see what parts of the end-to-end tests will
> > be slow and work on speeding some of them up and mocking others away.
> I've
> > fallen prey to premature optimization traps way too many times by now :)
> >
> > - Mark
> >
> >
> > On Thu, Nov 7, 2013 at 3:06 PM, Braden Shepherdson <braden@chromium.org
> > >wrote:
> >
> > > What we're aiming for here is that we mock as little as is practical,
> and
> > > ideally only mock things at the boundaries of CLI: calls to the
> platform
> > > scripts, some calls to Plugman (generally we want to let these
> end-to-end
> > > tests call the real Plugman, but there are exceptions).
> > >
> > > We'll do it first, and then solve the speed problems later if any
> arise.
> > > There are some options here, like I mentioned above: Use a fake fs
> > > implementation[1] or a ramdisk with (much!) better performance than the
> > > real disk. And just make CLI faster in general.
> > >
> > > Braden
> > >
> > > [1] https://github.com/eldargab/node-fake-fs
> > >
> > >
> > > On Thu, Nov 7, 2013 at 2:57 PM, Anis KADRI <anis.kadri@gmail.com>
> wrote:
> > >
> > > > The reason why every FS call is mocked is speed but speed is
> > > > subjective in my opinion. Given the features of CLI, my opinion is
> > > > that anything < 1 minute is acceptable. When I run tests, I am not
> > > > actively watching them execute.
> > > > I think the only calls that should be mocked are network calls
> because
> > > > you should be able to run tests offline.
> > > > I think we should convert (trash?) existing tests back to what they
> > > > were instead of adding more end-to-end tests. We should also pay more
> > > > attention de Windows.
> > > >
> > > > On Thu, Nov 7, 2013 at 10:31 AM, Braden Shepherdson <
> > braden@chromium.org
> > > >
> > > > wrote:
> > > > > Alright, Mark and I have discussed this further, and we will be
> > > beginning
> > > > > the effort with some end-to-end tests that will supplement the
> > existing
> > > > > tests.
> > > > >
> > > > > To some extent this is duplicating things that go on in the CI,
> since
> > > it
> > > > > checks out various plugins using the tools. But we think it's
> still a
> > > > > worthwhile effort since it will make it much easier to catch any
> > > problems
> > > > > at commit time.
> > > > >
> > > > > The main pain point in the existing tests that we'll want to fix
is
> > > their
> > > > > fragility. The worst offenders here are the platform parsers,
> > > especially
> > > > > wp7+8. So Steve, if you're looking for ways to help, I'd suggest
> > > looking
> > > > at
> > > > > those (wp in particular, but all of them). These are some of the
> > worst,
> > > > > most vacuous tests we have. They're not providing sample inputs and
> > > > > checking the outputs, they are checking that the right functions
> get
> > > > > called. I propose that they should be operating on a real,
> > checked-in,
> > > > > example project, in the spec/fixtures directory. The tests should
> run
> > > the
> > > > > parsers, and make sure all the files are in the right places and
> have
> > > the
> > > > > right contents.
> > > > >
> > > > > Braden
> > > > >
> > > > >
> > > > > On Thu, Nov 7, 2013 at 1:02 PM, Steve Gill <stevengill97@gmail.com
> >
> > > > wrote:
> > > > >
> > > > >> I don't think we should scrap the current tests. I am totally
in
> > favor
> > > > of
> > > > >> having new end to end tests. We need to catch regressions better.
> > > > >>
> > > > >> Braden, let me know how I can help.
> > > > >>
> > > > >>
> > > > >>
> > > > >> > On Nov 7, 2013, at 9:02 AM, Michal Mocny <mmocny@chromium.org>
> > > wrote:
> > > > >> >
> > > > >> > Discussing locally with Braden.. these tests seem to be
testing
> > > > internal
> > > > >> > details instead of expected functionality.  Its quite common
for
> > > valid
> > > > >> > patches to break the tests and invalid patches to leave
the
> tests
> > > > >> passing.
> > > > >> > There have been several occurrences recently where things
landed
> > > even
> > > > >> > though "cordova create" or "plugin add" were hosed, which
seems
> > > fairly
> > > > >> > fundamental to keep working ;)
> > > > >> >
> > > > >> >
> > > > >> >> On Thu, Nov 7, 2013 at 11:57 AM, Michal Mocny <
> > mmocny@chromium.org
> > > >
> > > > >> wrote:
> > > > >> >>
> > > > >> >> +1 to testing end-to-end, but I thought thats what
> BuildBot/Medic
> > > > does?
> > > > >> >> Likely we want to ship those end-to-end test scripts
so users
> can
> > > > test
> > > > >> >> changes locally, but I think the tests are are inside
CLI now
> are
> > > > meant
> > > > >> to
> > > > >> >> be unit tests, which yes, have very limited usefulness
in
> > > isolation,
> > > > but
> > > > >> >> perhaps shouldn't be entirely replaced?  If they really
are
> > useless
> > > > (not
> > > > >> >> sure) then they should be fixed/removed.
> > > > >> >>
> > > > >> >>
> > > > >> >> On Thu, Nov 7, 2013 at 11:40 AM, Braden Shepherdson
<
> > > > >> braden@chromium.org>wrote:
> > > > >> >>
> > > > >> >>> The CLI tests are bad. I propose making them better.
> > > > >> >>>
> > > > >> >>> The tests are bad for two reasons:
> > > > >> >>> 1. They're fragile because the tests depend on exactly
the
> right
> > > > >> functions
> > > > >> >>> being called, sometimes in the right order.
> > > > >> >>> 2. They don't test what we really want, which is
that projects
> > get
> > > > >> created
> > > > >> >>> and all the files are in the right places.
> > > > >> >>>
> > > > >> >>> I propose letting the tests actually run filesystem
and
> related
> > > > calls,
> > > > >> >>> instead of always mocking them out. In the simplest
form, that
> > > means
> > > > >> >>> running them on the real filesystem. If that's too
slow, we
> can
> > > > >> >>> investigate
> > > > >> >>> other alternatives, like using a ramdisk, or using
that
> emulated
> > > fs
> > > > >> that
> > > > >> >>> runs everything in RAM inside Node.
> > > > >> >>>
> > > > >> >>> Mark and I are planning to work on this, starting
with the CLI
> > > > tests.
> > > > >> The
> > > > >> >>> Plugman tests are better in this regard, but could
probably
> > stand
> > > > to be
> > > > >> >>> really called as well.
> > > > >> >>>
> > > > >> >>> Any thoughts or objections?
> > > > >> >>>
> > > > >> >>> Braden
> > > > >> >>
> > > > >> >>
> > > > >>
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message