I see no situation where we don't want a feature branch vetted by >1 person before we land anything on master …short of fixing broken tests.
I assume good faith. Joe: you had a bad day and, I think, you still feel mistrust after previous commits landing on master stalling out your work last summer. Lets put that behind us. Andrew pls fire a ping to the list w/ a PR for anything aiming to live on Android master until earn Joe's trust back. And lets avoid the editorial about motivations. We all want the same thing here: work on a kick ass open source project that makes a difference. On Thu, Feb 12, 2015 at 9:31 AM, Jesse <purplecabb...@gmail.com> wrote: > This commit may not have warranted this discussion. > I think we agree that large changes/commits should be on feature branches, > and discussed before being merged. > Let's go with that. > > > > > On Feb 12, 2015, at 8:49 AM, Andrew Grieve <agri...@chromium.org> wrote: > > > > Sounds like you've been having a rough time. :( Hope you get through it. > > > > Believe me when I say I hear you loud and clear about making changes on > > feature branches. I just don't think this one fits. > > - No one (other than me) has touched the tests since September of last > > year, so it's unlikely that a change would cause merge conflicts. > > - The state of the tests show that they are not viewed as that important > > (at least not important enough for anyone other than me to have been > > working on them) > > - Anything I do to them doesn't affect shipping code. No risk. > > > > I find it hard to believe that my making changes contributes in a > > significant way to having people avoid working on Android. Perhaps being > > overly abrasive via email & JIRA would be a deterrent though... > > > > > > > > > >> On Thu, Feb 12, 2015 at 11:10 AM, Joe Bowser <bows...@gmail.com> wrote: > >> > >> On Thu Feb 12 2015 at 7:44:52 AM Andrew Grieve <agri...@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 <purplecabb...@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 <agri...@chromium.org> > >>> wrote: > >>>>> > >>>>>> On Wed, Feb 11, 2015 at 5:00 PM, Jesse <purplecabb...@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 <bows...@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 <bows...@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 < > >>> agri...@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 < > >>> mura...@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: agri...@google.com [mailto:agri...@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-unsubscr...@cordova.apache.org > >>>>>>>>>> For additional commands, e-mail: dev-h...@cordova.apache.org > >>>> > >>>> --------------------------------------------------------------------- > >>>> To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org > >>>> For additional commands, e-mail: dev-h...@cordova.apache.org > >> > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org > For additional commands, e-mail: dev-h...@cordova.apache.org > >
