flex-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From OmPrakash Muppirala <bigosma...@gmail.com>
Subject Re: [JIRA](ISSUE #FLEX-34119) Dev review request
Date Fri, 08 Aug 2014 17:47:27 GMT
Our code is mirrored almost in real-time on to github.  Github has
excellent code review features.  We can use that to do our post-commit code
reviews.

Here are the steps:
1.  Go to https://github.com/apache/flex-sdk
2.  In the 'branch' dropdown, select the branch: FLEX-34119
3.  Click on the button labeled "xxx commits".  It should show you all the
recent commits
4.  You can click on each commit to see its comments and code changes as a
diff.  For example:
https://github.com/apache/flex-sdk/commit/2f1b76827763b32988b1a1ccc73e603d9d7d788a
5.  When you mouse over each line, you will see a 'comment bubble' button
show up on the left.  Click on it and enter your comments for that line.

P.S.  I am currently working with INFRA to make these comments to show up
on the dev@f.a.o list automatically.  Until then, commenters/reviewers can
post on the dev list manually indicating that they have reviewed a commit.

Hope this helps.

Thanks,
Om


On Fri, Aug 8, 2014 at 9:32 AM, Frédéric THOMAS <webdoublefx@hotmail.com>
wrote:

> For Crucible (code review), nothing we can do on our side, only INFRA
> could request it and I didn't ask them to do so (if you want to ask, it's
> up to you, it would be a very nice feature but you don't be in hurry), only
> FishEye has been setup while ago
> https://fisheye6.atlassian.com/browse/flex-sdk
> Frédéric THOMAS
>
> > From: mihai.chira@gmail.com
> > Date: Fri, 8 Aug 2014 17:21:04 +0100
> > Subject: Re: [JIRA](ISSUE #FLEX-34119) Dev review request
> > CC: dev@flex.apache.org
> >
> > Frédéric, have you found out anything else regarding FishEye? I took a
> > look, but the Flex project isn't there (in fact, there seems to be
> > only one project, called "Default Project". It would be great to
> > review each other's code using this.
> >
> > Piotr, have you committed the TLF automatic test running code yet? It
> > would be great to run the test automatically in flex too, not least
> > because while trying to solve FLEX-34119 I had to create five separate
> > unit tests.
> >
> > Alex, I think I've finally nailed FLEX-34119! Since I first wrote
> > about it, it morphed into a total of five bugs: FLEX-34440,
> > FLEX-34424, FLEX-34456, FLEX-34458, and the original FLEX-34119. All
> > of them have unit tests, and now they all pass, which took a while.
> > (Wow, I have to say, HierarchicalCollectionViewCursor was pretty
> > broken.)
> >
> > There is a serious amount of code now on the FLEX-34119 branch[1]. It
> > would really help if you (and potentially others) could take another
> > look when you have time. Each commit has extensive information for
> > what was wrong, and what the solution is. Let me know if you think any
> > of those bugs could be solved in other ways. (Note that there are 7 or
> > 8 commits which are not mine; they were introduced when I merged with
> > develop.)
> >
> >
> > [1]
> https://fisheye6.atlassian.com/graph/flex-sdk#branch=FLEX-34119&hl=Lineage&scrollToCsid=82d6b51694e846f0fb4e5505bf5a1b0601c21e7b
> >
> > On 22 July 2014 21:34, Alex Harui <aharui@adobe.com> wrote:
> > > Regarding code review, we do have tools but for most changes, if the
> tests pass you check it in and folks review the commit email.
> > > Sent via the PANTECH Discover, an AT&T 4G LTE smartphone.
> > >
> > > Mihai Chira <mihai.chira@gmail.com> wrote:
> > >
> > >
> > > Hi Alex,
> > >
> > >
> > > thanks for taking a look. I was also surprised to discover not only
> > > that the parentTable variable wasn't necessary, but also that two
> > > entire if branches (for add and remove) *always* ended up throwing the
> > > 'Bookmark no longer valid' error (see the commit message here[1] for
> > > details). In other words, they had never been tested.
> > >
> > > If there is some edge case, when we discover it we'll add a unit test
> > > to make sure it doesn't return.
> > > Speaking of which, I've just added the setItemAt operation to the unit
> > > test, and I got some more 'Bookmark no longer valid' errors. I'll fix
> > > these new occurrences hopefully tomorrow. Once I do and once I run the
> > > mustella tests, I'll ask you to take another look before I merge with
> > > develop.
> > >
> > > Two questions:
> > >
> > > 1. Would it be very complicated to run unit tests automatically in the
> > > ant build script, or to the server CI process?
> > > 2. Also, how do we do code reviews? Do we use some tool, like fisheye
> > > or github, or just the IDE? I didn't find a wiki page about that.
> > >
> > >
> > > Thanks,
> > > Mihai
> > >
> > >
> > > [1]
> https://fisheye6.atlassian.com/changelog/flex-sdk?cs=4cb80bfbb37225b86b9e69cc4ec2fc21efe0a668
> > >
> > > On 22 July 2014 17:49, Alex Harui <aharui@adobe.com> wrote:
> > >> Hi Mihai,
> > >>
> > >> Thanks for taking on such a challenging goal.
> > >>
> > >> I don't see anything obviously wrong.  IMO, the best thing to do
> would be
> > >> to set up mustella and run the Advanced DataGrid tests.  That's what
> our
> > >> CI system will do when you finally check this into the develop branch.
> > >>
> > >> It is a bit puzzling that the old code messed around with the parent
> stack
> > >> and it looks like you no longer need to.  I wonder if there is some
> edge
> > >> case associated with that.
> > >>
> > >> -Alex
> > >>
> > >> On 7/22/14 4:00 AM, "Mihai Chira" <mihai.chira@gmail.com> wrote:
> > >>
> > >>>On the branch "FLEX-34119" I committed two unit tests and a fix for
> > >>>this bug. If anyone would like to take a look, and run the tests, it
> > >>>would be really helpful, because it's a trickier area of the SDK.
> > >>>Could you let me know if:
> > >>>* you think the unit tests are in the right folder
> > >>>* the unit tests run as expected (see the README in
> > >>>HierarchicalCollectionViewCursor_FLEX_34119_Test for some explanations
> > >>>about it)
> > >>>* it looks like the commits might break anything else
> > >>>* you think the variable renames in HierarchicalCollectionViewCursor
> are
> > >>>helpful
> > >>>* we could run the unit tests automatically on each ant build. Is that
> > >>>possible now?
> > >>>
> > >>>I'm still adding an operation to the unit test (setItemAt), and
> > >>>looking to remove some duplicated code from
> > >>>HierarchicalCollectionViewCursor.collectionChangeHandler.
> > >>>I also plan to run the mustella tests soon, to make sure I haven't
> > >>>broken anything.
> > >>>
> > >>>
> > >>>Thank you!
> > >>>Mihai
> > >>
>
>

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