cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Bowser <bows...@gmail.com>
Subject Re: Android JUnit Tests Now Pass
Date Thu, 12 Feb 2015 16:10:18 GMT
On Thu Feb 12 2015 at 7:44:52 AM Andrew Grieve <agrieve@chromium.org> wrote:

> I agree that significant changes should be reviewed first. But for the most
> part Cordova is a review-after-commit kind of place,


No, it's not.  Cordova is only like that because you consistently make it
like that.  Constantly committing to master without any review at all makes
it next to impossible for anyone else to work on the project.  You can tell
that this is the case, because you own the majority of the commits over the
last few months. That's not normal for a codebase this size with this many
contributors.  This is why we have topic branches, and we've had this
discussion with you numerous times about using them.  This is also why I
write e-mails trying to get buy-in to what I want to do instead of just
landing features straight on master that could break everything.


> and this change didn't
> touch any code that we release (strictly tests... that have been broken for
> a very long time), so I don't think it qualifies.
>
>
I'll admit that the tests were a bit of the wild west.  That said, there
was always an understanding that tests would be added to and improved upon
and not removed.  Marcel and I probably wouldn't have had half the e-mails
that we have had if it wasn't us arguing over whether to delete tests.

I know it's frustrating to have to wait on other people, since people are
human, get sick, and have to take care of others when they're sick.  That
said, it's equally frustrating to come back from vacation, or wake up from
a nap after driving someone from the hospital and see that critical code
that was a major issue only six months ago got accidentally removed in a
sweeping change, along with other use cases.  That's what happened
yesterday, and that's why I got frustrated.  If I'm having a bad day
already, a random refactor that just gets dropped without at least a head's
up beforehand makes it worse.

I've been on both sides of the issue with this.  I remember getting
extremely frustrated with Bryce when we designed CordovaWebView, especially
since my design had less of a change to the code.  I thought things were
moving too slowly, but at the end of the day we did produce something that
a lot of people seem to use (at least that's what the sample repo I have on
GitHub tells me, the GitHub analytics tools are all I have to go on). That
said, we didn't ship that until it was mostly ready, and other than an
awkward presentation at PhoneGap Day, it was mostly fine.  I'm glad I
didn't just merge my crap in and just unilaterally introduce that feature,
since back then we could still get away with that technically.

But yeah, can we have things on feature branches if they're that big, and
then wait maybe 24 hours before dropping something like that? I'm not
talking like a simple JIRA fix, but something that large should have been a
pull request or on its own branch or something.


> On Thu, Feb 12, 2015 at 4:07 AM, Jesse <purplecabbage@gmail.com> wrote:
>
> > You may or may not, but I think it would be nice to let others review
> your
> > (significant) changes before dumping them to master.
> >
> >
> > > On Feb 11, 2015, at 6:34 PM, Andrew Grieve <agrieve@chromium.org>
> wrote:
> > >
> > >> On Wed, Feb 11, 2015 at 5:00 PM, Jesse <purplecabbage@gmail.com>
> wrote:
> > >>
> > >> +1 Revert
> > >>
> > >> And please let's stop deleting what other people wrote just because we
> > >> don't recognize it. These things should require discussion.
> > >
> > > Bit of a jump to conclusions, don't you think? What makes you think I
> > don't
> > > recognize the code I changed?
> > >
> > >
> > >>
> > >> @purplecabbage
> > >> risingj.com
> > >>
> > >>> On Wed, Feb 11, 2015 at 1:53 PM, Joe Bowser <bowserj@gmail.com>
> wrote:
> > >>>
> > >>> I think we should revert this refactor.  With the new refactored
> tests,
> > >>> they may pass but we lost a lot of the useful tests that we once had
> > and
> > >>> these new tests have no value.  I don't know why you took it upon
> > >> yourself
> > >>> to throw away all the JUnit tests that didn't pass, but that misses
> the
> > >>> point.  I would have rather had the old tests expanded upon instead
> of
> > >> just
> > >>> deleted on your personal whim.
> > >>>
> > >>> I honestly don't know what to say, I know that we have a terrible
> > working
> > >>> relationship at best, but this actually is making the project worse
> > >>> intentionally for unknown reasons.  In fact, I would almost say that
> > this
> > >>> is purely a malicious change driven by ego, since I can't see a
> > technical
> > >>> reason for any of it.
> > >>>
> > >>>> On Wed Feb 11 2015 at 1:36:19 PM Joe Bowser <bowserj@gmail.com>
> > wrote:
> > >>>>
> > >>>> I think there's a lot of value in the Unit Tests, having wrote
the
> > >>>> majority of them initially.  If I wasn't dealing with everyone
in my
> > >>> house
> > >>>> getting sick, I'd check to make sure these tests were still testing
> > >> what
> > >>> I
> > >>>> intended them to test, since we have a habit of losing the intent
> > >> behind
> > >>>> the test every time we do a refactor.
> > >>>>
> > >>>> Of course, if we're going to throw away the embedded WebView case,
> > then
> > >>>> maybe there's not value after all.
> > >>>>
> > >>>> On Wed Feb 11 2015 at 1:12:29 PM Andrew Grieve <
> agrieve@chromium.org>
> > >>>> wrote:
> > >>>>
> > >>>>> Does travis provide Android emulators? I'd guess it'd be too
slow
> to
> > >> put
> > >>>>> on
> > >>>>> Travis. And honestly, there's still not a lot of value in the
unit
> > >> tests
> > >>>>> atm.
> > >>>>>
> > >>>>> On Wed, Feb 11, 2015 at 3:12 PM, Murat Sutunc <
> muratsu@microsoft.com
> > >
> > >>>>> wrote:
> > >>>>>
> > >>>>>> This is great news!
> > >>>>>> I've finally got the android travis enabled too. We have
jshint
> and
> > >>>>>> jasmine test coverage on every commit now. (
> > >>>>>> https://travis-ci.org/apache/cordova-android/builds/50295748)
> > >>>>>>
> > >>>>>> Now that we're passing all junit tests, I think the next
step for
> us
> > >>>>>> should be to integrate junit tests with travis. What do
you think?
> > >>>>>>
> > >>>>>> -----Original Message-----
> > >>>>>> From: agrieve@google.com [mailto:agrieve@google.com] On
Behalf Of
> > >>>>> Andrew
> > >>>>>> Grieve
> > >>>>>> Sent: Tuesday, February 10, 2015 7:14 PM
> > >>>>>> To: dev
> > >>>>>> Subject: Android JUnit Tests Now Pass
> > >>>>>>
> > >>>>>> Spent some time cleaning up the tests. Certainly they could
be
> made
> > >>> even
> > >>>>>> better & made to test more things, but at least they
pass now :)
> > >>>>>>
> > >>>>>> Much of the change was deleting copy & paste, and deleting
> commented
> > >>> out
> > >>>>>> tests:
> > >>>>>> 53 files changed, 941 insertions(+), 2610 deletions(-)
> > >> ---------------------------------------------------------------------
> > >>>>>> To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
> > >>>>>> For additional commands, e-mail: dev-help@cordova.apache.org
> > >>
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
> > For additional commands, e-mail: dev-help@cordova.apache.org
> >
> >
>

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