Skip to main content

Code reviews for the recent "Test Export" updates

11 replies [Last post]
Anonymous

Hi Dmitri, team,

Let's devote this thread to the code reviews for the latest updates
for "Test Export" feature.

Let's discuss first what exactly should be reviewed, there were lots
of changes, in various branches, and even multiple bugfixes in the
single branches, with some merges between the branches.

Frankly, I'm lost a little bit.

Dmitri, you've done A LOT :) Let's sort it out and make it easier to
review.

Typically, I'd suggest to create a separate branch for every bugfix,
and make the separated bugfixes as independent from each other as
possible. I understand that this is not always possible.

But it is pretty hard for everybody, to have multiple bugfixes in the
single branch.

At the moment, my understanding is that there are 4 different issues
that were (are being?) fixed:

- Export of distributed tests
- Issue #60: In Test Export mode, not all copied sources are listed.
- Issue #57: Test Export creates and uses src.map file
- Improved export of test sources (or is this part of ISSUE #60?)
- Issue #38: Export directory is overwritten without warnings

Am I missing something, were there any other bugfixes?

I took a look at the some changes in the Dmitri's branch, but there
are so many of them, and I'm afraid that I could spend quite some
time trying to review something that's not needed to be reviewed...

Dmitri, would it be possible for you to create independent branches
for every bugfix, with clear bug id associated with every change, with
bugs' evaluations updated with information about the bugfix (what was
done and why), and post the relevant info into this thread?

I'm really sorry, that's a bit of extra work for you. Let me know if
the request is not feasible.

>From my side, I promise to devote one full day reviewing all your
changes, once you provide the list of bugfixes, with updated bug
reports, with separated branches.

Deal? :)

Given the amount of the code changes, I think that there should be
some unit tests provided. Some time ago we discussed unit tests policy
and my understanding is that we should strive to provide unit tests
for every new feature, especially for such big changes.

(Oh, I just noticed that you do provide some unit test, for new parser
functionality. But do we need to provide some more unit tests for all
other changes?).

Also, please note that some areas must be reviewed by Stan, and we
cannot commit them without his approval (packages com.sun.tck.cldc and
com.sun.cldc). In your case, TestBundler must definitely be reviewed
by Stan. The sooner you submit such CLDC-related changes for his
review, the sooner you get the response.. :)

Thanks,
--Vladimir

P.S. With big updates, it's always hard to properly review them, so it
would be great to find a balance somehow, and provide changes in more
iterative process, with a list of smaller fixes rather then with one
huge change. But again, it's not always easy to figure this out.

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Reply viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
Dmitri Trounine

Hi, Vladimir,

please review new Javadoc commets for my last update. They are contained
in two changesets:

http://fisheye4.cenqua.com/changelog/cqme/?cs=518
http://fisheye4.cenqua.com/changelog/cqme/?cs=519

The updated sources:

http://fisheye4.cenqua.com/browse/cqme/branches/users/dtrounine/testExpo...

Thanks,
Dmitri.

Vladimir Sizikov wrote:

>Once you've provided the javadocs, please commit.
>We can deal with smaller issues by filing appropriate bugs.
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Vladimir Sizikov

Hi Dmitri,

There is on small problem in javadocs:
http://fisheye4.cenqua.com/changelog/cqme/?cs=518

259 + * adding -set &ls;exported.test.question.tag>=&ls;test bundle&gt
260 + *
argument.

is rendered incorrectly. Maybe, we should not use '<' and '>' brackets at
all here?

And one more general note. In our javadocs we tend to use
"Returns ..." for getXXX methods, while in all your new javadocs you
use "Gets ...". I like consistency. :)

So, there are three options here:

1. Replace all Gets to Returns and then commit.
2. Commit, and then replace Gets to Returns in entire repository.
3. Just commit and do nothing.

What do you think? Please pick what you think is the best choice here.

I looked at Java Platform jadadocs, and it seems that "Gets" is out of
fashion, preserved only in the older APIs, while all the new APIs
consistently use "Returns".

Thanks,
--Vladimir

On Tue, Mar 20, 2007 at 12:26:52PM +0300, Dmitri Trounine wrote:
> Hi, Vladimir,
>
> please review new Javadoc commets for my last update. They are contained
> in two changesets:
>
> http://fisheye4.cenqua.com/changelog/cqme/?cs=518
> http://fisheye4.cenqua.com/changelog/cqme/?cs=519
>
> The updated sources:
>
> http://fisheye4.cenqua.com/browse/cqme/branches/users/dtrounine/testExpo...
>
> Thanks,
> Dmitri.
>
> Vladimir Sizikov wrote:
>
> >Once you've provided the javadocs, please commit.
> >We can deal with smaller issues by filing appropriate bugs.
> >
> >
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Dmitri Trounine

Hi Valdimir,

thanks again for your comments. I've fixed all errors found in Javadoc
info in my update. Sure, there may be more errors or missed Javadoc
comments... Let fix it case by case in next updates?

Changeset: http://fisheye4.cenqua.com/changelog/cqme/?cs=521

Updated sources:
https://cqme.dev.java.net/source/browse/cqme/branches/users/dtrounine/te...
https://cqme.dev.java.net/source/browse/cqme/branches/users/dtrounine/te...
https://cqme.dev.java.net/source/browse/cqme/branches/users/dtrounine/te...
https://cqme.dev.java.net/source/browse/cqme/branches/users/dtrounine/te...
https://cqme.dev.java.net/source/browse/cqme/branches/users/dtrounine/te...
https://cqme.dev.java.net/source/browse/cqme/branches/users/dtrounine/te...
https://cqme.dev.java.net/source/browse/cqme/branches/users/dtrounine/te...
https://cqme.dev.java.net/source/browse/cqme/branches/users/dtrounine/te...

Vladimir Sizikov wrote:

>259 + * adding -set &ls;exported.test.question.tag>=&ls;test bundle&gt
>260 + *
argument.
>
>is rendered incorrectly. Maybe, we should not use '<' and '>' brackets at
>all here?
>
>
>
Replaced &ls; by <

>And one more general note. In our javadocs we tend to use
>"Returns ..." for getXXX methods, while in all your new javadocs you
>use "Gets ...". I like consistency. :)
>
>So, there are three options here:
>
>1. Replace all Gets to Returns and then commit.
>
>
I select this choice.

>I looked at Java Platform jadadocs, and it seems that "Gets" is out of
>fashion, preserved only in the older APIs, while all the new APIs
>consistently use "Returns".
>
>
>
I looked at Javatest javadocs, and they use "Get" (even not "Gets"). I
am not saying that we shoud follow their policy, it's just an
observation. By the way, is there a policy for writing Javadoc comments
in our project? Can we use
http://java.sun.com/j2se/javadoc/writingdoccomments/index.html ?

Thanks,
Dmitri.

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Vladimir Sizikov

Dmitri,

Lots of good work, thanks!
Please commit while it's hot. :)

--Vladimir

On Tue, Mar 20, 2007 at 06:09:45PM +0300, Dmitri Trounine wrote:
> Hi Valdimir,
>
> thanks again for your comments. I've fixed all errors found in Javadoc
> info in my update. Sure, there may be more errors or missed Javadoc
> comments... Let fix it case by case in next updates?
>
> Changeset: http://fisheye4.cenqua.com/changelog/cqme/?cs=521
>
> Updated sources:
> https://cqme.dev.java.net/source/browse/cqme/branches/users/dtrounine/te...
> https://cqme.dev.java.net/source/browse/cqme/branches/users/dtrounine/te...
> https://cqme.dev.java.net/source/browse/cqme/branches/users/dtrounine/te...
> https://cqme.dev.java.net/source/browse/cqme/branches/users/dtrounine/te...
> https://cqme.dev.java.net/source/browse/cqme/branches/users/dtrounine/te...
> https://cqme.dev.java.net/source/browse/cqme/branches/users/dtrounine/te...
> https://cqme.dev.java.net/source/browse/cqme/branches/users/dtrounine/te...
> https://cqme.dev.java.net/source/browse/cqme/branches/users/dtrounine/te...
>
>
> Vladimir Sizikov wrote:
>
> >259 + * adding -set &ls;exported.test.question.tag>=&ls;test
> >bundle&gt
> >260 + *
argument.
> >
> >is rendered incorrectly. Maybe, we should not use '<' and '>' brackets at
> >all here?
> >
> >
> >
> Replaced &ls; by <
>
> >And one more general note. In our javadocs we tend to use
> >"Returns ..." for getXXX methods, while in all your new javadocs you
> >use "Gets ...". I like consistency. :)
> >
> >So, there are three options here:
> >
> >1. Replace all Gets to Returns and then commit.
> >
> >
> I select this choice.
>
> >I looked at Java Platform jadadocs, and it seems that "Gets" is out of
> >fashion, preserved only in the older APIs, while all the new APIs
> >consistently use "Returns".
> >
> >
> >
> I looked at Javatest javadocs, and they use "Get" (even not "Gets"). I
> am not saying that we shoud follow their policy, it's just an
> observation. By the way, is there a policy for writing Javadoc comments
> in our project? Can we use
> http://java.sun.com/j2se/javadoc/writingdoccomments/index.html ?
>
> Thanks,
> Dmitri.

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Dmitri Trounine

Hi guys,

I have just prepared a number of updates for review. Each update is in
separate branch and is easily browsable...

1) The first and the biggest is the update which enables export of
distributed tests. Vladimir is going to review this update, and Stan
should take a look at changes in com.sun.tck.cldc.javatest package.
Anyone else is also welcome to review this update.
In order to start to review this update, please read my overview of this
update on wiki:

http://wiki.java.net/bin/view/Mobileandembedded/MEFrameworkTestExportDis...

It contains links to diffs and modified files in my private branch. If
you want to checkout this update, you need the following revision (not
HEAD!):

https://cqme.dev.java.net/svn/cqme/branches/users/dtrounine/testExport/m...

2) Then, a number of minor updates follow:

http://forums.java.net/jive/thread.jspa?threadID=24112 The most
interesting to review
http://forums.java.net/jive/thread.jspa?threadID=24111 The easiest
to review
http://forums.java.net/jive/thread.jspa?threadID=24110 Not too
hard to review

All are welcome to review!

Thanks,
Dmitri.

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Vladimir Sizikov

Hi Dmitri,

I reviewed the change #1 in the series of test export updates (the
biggest one).

Here are my comments:

First, a general one. New features often confuse long-time users, and
with addition of "run export tests" in the Interview, I'm a bit
confused too. What's the use case for that? I can admit, that this
functionality is quite cool, but would anybody use it regularly?

Sure, the moment I saw this choice, I selected it, and started test
run, and then started MIDP RI in autotest mode to see what happens.
Well, HTTP error 405 happens :) It took me some time to realize that I
should not start MIDP RI in autotest mode, but I was confused for a
while. Should we document this in more detail so that users understand
what's going on? At a minimum, "More Info" for Test Export should be
greatly expanded.

More specific comments:

0. Slow performance when exporting the tests. I noticed that the
test export is quite slow now. Is there anything that can be
improved here? Do you see this problem in your setup?

1. Javadocs needed for newly added methods:
- TestExportInfo
- TestBundler (esp. here!)
- ExportTetsSuite
- probably more in all other changed sources

Please consider adding proper javadoc comments to all newly added
APIs: public classes, public and protected methods, protected
fields.

We don't have much of development documentation and rely on proper
javadocs for that. Proper documentation for the new features will
simplify life of everybody who will be maintaining these features.

2. ExportedBundlingService.java - most of the file is commented out.
Shouldn't it be removed? Also, should probably be removed?:
// return new ExportedTestProvider(agentClass, jarSourceDir);

3. More Info for "Export Mode" should be extended, providing
explanation for "Run exported tests", in greater detail. This is
new behavior and new users will find it a bit confusing. GUI and
batch mode most probably should be explained as well. Some of the
content from the wiki can be used here, but remember - More Info is
for the users, not for the developers. :)

4. ant target "run-test" is confusing for non-distributed tests, it
just returns OK. Confusing for new users.

Once you've provided the javadocs, please commit.
We can deal with smaller issues by filing appropriate bugs.

Thanks,
--Vladimir

On Tue, Mar 13, 2007 at 04:11:35PM +0300, Dmitri Trounine wrote:
> Hi guys,
>
> I have just prepared a number of updates for review. Each update is in
> separate branch and is easily browsable...
>
> 1) The first and the biggest is the update which enables export of
> distributed tests. Vladimir is going to review this update, and Stan
> should take a look at changes in com.sun.tck.cldc.javatest package.
> Anyone else is also welcome to review this update.
> In order to start to review this update, please read my overview of this
> update on wiki:
>
> http://wiki.java.net/bin/view/Mobileandembedded/MEFrameworkTestExportDis...
>
> It contains links to diffs and modified files in my private branch. If
> you want to checkout this update, you need the following revision (not
> HEAD!):
>
> https://cqme.dev.java.net/svn/cqme/branches/users/dtrounine/testExport/m...
>
> 2) Then, a number of minor updates follow:
>
> http://forums.java.net/jive/thread.jspa?threadID=24112 The most
> interesting to review
> http://forums.java.net/jive/thread.jspa?threadID=24111 The easiest
> to review
> http://forums.java.net/jive/thread.jspa?threadID=24110 Not too
> hard to review
>
> All are welcome to review!
>
> Thanks,
> Dmitri.
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Dmitri Trounine

Hi Vladimir,

Thanks for review. I'll add Javadoc info to my code and will commit.
Then I'll file issues that you have mentioned and we will proceed with
new fixes and reviews (including low performance problem). Ok?

Thanks,
Dmitri.

Vladimir Sizikov wrote:

>Hi Dmitri,
>
>I reviewed the change #1 in the series of test export updates (the
>biggest one).
>
>Here are my comments:
>
>First, a general one. New features often confuse long-time users, and
>with addition of "run export tests" in the Interview, I'm a bit
>confused too. What's the use case for that? I can admit, that this
>functionality is quite cool, but would anybody use it regularly?
>
>Sure, the moment I saw this choice, I selected it, and started test
>run, and then started MIDP RI in autotest mode to see what happens.
>Well, HTTP error 405 happens :) It took me some time to realize that I
>should not start MIDP RI in autotest mode, but I was confused for a
>while. Should we document this in more detail so that users understand
>what's going on? At a minimum, "More Info" for Test Export should be
>greatly expanded.
>
>More specific comments:
>
>0. Slow performance when exporting the tests. I noticed that the
> test export is quite slow now. Is there anything that can be
> improved here? Do you see this problem in your setup?
>
>1. Javadocs needed for newly added methods:
> - TestExportInfo
> - TestBundler (esp. here!)
> - ExportTetsSuite
> - probably more in all other changed sources
>
> Please consider adding proper javadoc comments to all newly added
> APIs: public classes, public and protected methods, protected
> fields.
>
> We don't have much of development documentation and rely on proper
> javadocs for that. Proper documentation for the new features will
> simplify life of everybody who will be maintaining these features.
>
>2. ExportedBundlingService.java - most of the file is commented out.
> Shouldn't it be removed? Also, should probably be removed?:
> // return new ExportedTestProvider(agentClass, jarSourceDir);
>
>3. More Info for "Export Mode" should be extended, providing
> explanation for "Run exported tests", in greater detail. This is
> new behavior and new users will find it a bit confusing. GUI and
> batch mode most probably should be explained as well. Some of the
> content from the wiki can be used here, but remember - More Info is
> for the users, not for the developers. :)
>
>4. ant target "run-test" is confusing for non-distributed tests, it
> just returns OK. Confusing for new users.
>
>Once you've provided the javadocs, please commit.
>We can deal with smaller issues by filing appropriate bugs.
>
>Thanks,
> --Vladimir
>
>On Tue, Mar 13, 2007 at 04:11:35PM +0300, Dmitri Trounine wrote:
>
>
>>Hi guys,
>>
>>I have just prepared a number of updates for review. Each update is in
>>separate branch and is easily browsable...
>>
>>1) The first and the biggest is the update which enables export of
>>distributed tests. Vladimir is going to review this update, and Stan
>>should take a look at changes in com.sun.tck.cldc.javatest package.
>>Anyone else is also welcome to review this update.
>>In order to start to review this update, please read my overview of this
>>update on wiki:
>>
>>http://wiki.java.net/bin/view/Mobileandembedded/MEFrameworkTestExportDistributedOverview
>>
>>It contains links to diffs and modified files in my private branch. If
>>you want to checkout this update, you need the following revision (not
>>HEAD!):
>>
>>https://cqme.dev.java.net/svn/cqme/branches/users/dtrounine/testExport/mergedDistributedTests@419
>>
>>2) Then, a number of minor updates follow:
>>
>> http://forums.java.net/jive/thread.jspa?threadID=24112 The most
>>interesting to review
>> http://forums.java.net/jive/thread.jspa?threadID=24111 The easiest
>>to review
>> http://forums.java.net/jive/thread.jspa?threadID=24110 Not too
>>hard to review
>>
>>All are welcome to review!
>>
>>Thanks,
>> Dmitri.
>>
>>
>>
>>---------------------------------------------------------------------
>>To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
>>For additional commands, e-mail: meframework-help@cqme.dev.java.net
>>
>>
>>
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
>For additional commands, e-mail: meframework-help@cqme.dev.java.net
>
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Vladimir Sizikov

Hi Dmitri,

On Mon, Mar 19, 2007 at 07:07:20PM +0300, Dmitri Trounine wrote:
> Thanks for review. I'll add Javadoc info to my code and will commit.
> Then I'll file issues that you have mentioned and we will proceed with
> new fixes and reviews (including low performance problem). Ok?

Sounds good. Please make sure you filed all the appropriate bugs in
the Issue Tracker, I will definitely forget them if no bugs are in the
database, too many things happen simultaneously, and it's hard to keep
all the issues in my little head.

Btw, just to make sure we are in sync, I reviewed all your proposed
bugfixes and responded in separete threads. Please let me know if I
missed anything.

Thanks,
--Vladimir

> Vladimir Sizikov wrote:
>
> >Hi Dmitri,
> >
> >I reviewed the change #1 in the series of test export updates (the
> >biggest one).
> >
> >Here are my comments:
> >
> >First, a general one. New features often confuse long-time users, and
> >with addition of "run export tests" in the Interview, I'm a bit
> >confused too. What's the use case for that? I can admit, that this
> >functionality is quite cool, but would anybody use it regularly?
> >
> >Sure, the moment I saw this choice, I selected it, and started test
> >run, and then started MIDP RI in autotest mode to see what happens.
> >Well, HTTP error 405 happens :) It took me some time to realize that I
> >should not start MIDP RI in autotest mode, but I was confused for a
> >while. Should we document this in more detail so that users understand
> >what's going on? At a minimum, "More Info" for Test Export should be
> >greatly expanded.
> >
> >More specific comments:
> >
> >0. Slow performance when exporting the tests. I noticed that the
> > test export is quite slow now. Is there anything that can be
> > improved here? Do you see this problem in your setup?
> >
> >1. Javadocs needed for newly added methods:
> > - TestExportInfo
> > - TestBundler (esp. here!)
> > - ExportTetsSuite
> > - probably more in all other changed sources
> >
> > Please consider adding proper javadoc comments to all newly added
> > APIs: public classes, public and protected methods, protected
> > fields.
> >
> > We don't have much of development documentation and rely on proper
> > javadocs for that. Proper documentation for the new features will
> > simplify life of everybody who will be maintaining these features.
> >
> >2. ExportedBundlingService.java - most of the file is commented out.
> > Shouldn't it be removed? Also, should probably be removed?:
> > // return new ExportedTestProvider(agentClass, jarSourceDir);
> >
> >3. More Info for "Export Mode" should be extended, providing
> > explanation for "Run exported tests", in greater detail. This is
> > new behavior and new users will find it a bit confusing. GUI and
> > batch mode most probably should be explained as well. Some of the
> > content from the wiki can be used here, but remember - More Info is
> > for the users, not for the developers. :)
> >
> >4. ant target "run-test" is confusing for non-distributed tests, it
> > just returns OK. Confusing for new users.
> >
> >Once you've provided the javadocs, please commit.
> >We can deal with smaller issues by filing appropriate bugs.
> >
> >Thanks,
> > --Vladimir
> >
> >On Tue, Mar 13, 2007 at 04:11:35PM +0300, Dmitri Trounine wrote:
> >
> >
> >>Hi guys,
> >>
> >>I have just prepared a number of updates for review. Each update is in
> >>separate branch and is easily browsable...
> >>
> >>1) The first and the biggest is the update which enables export of
> >>distributed tests. Vladimir is going to review this update, and Stan
> >>should take a look at changes in com.sun.tck.cldc.javatest package.
> >>Anyone else is also welcome to review this update.
> >>In order to start to review this update, please read my overview of this
> >>update on wiki:
> >>
> >>http://wiki.java.net/bin/view/Mobileandembedded/MEFrameworkTestExportDistributedOverview
> >>
> >>It contains links to diffs and modified files in my private branch. If
> >>you want to checkout this update, you need the following revision (not
> >>HEAD!):
> >>
> >>https://cqme.dev.java.net/svn/cqme/branches/users/dtrounine/testExport/mergedDistributedTests@419
> >>
> >>2) Then, a number of minor updates follow:
> >>
> >> http://forums.java.net/jive/thread.jspa?threadID=24112 The most
> >>interesting to review
> >> http://forums.java.net/jive/thread.jspa?threadID=24111 The easiest
> >>to review
> >> http://forums.java.net/jive/thread.jspa?threadID=24110 Not too
> >>hard to review
> >>
> >>All are welcome to review!
> >>
> >>Thanks,
> >> Dmitri.
> >>
> >>
> >>
> >>---------------------------------------------------------------------
> >>To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> >>For additional commands, e-mail: meframework-help@cqme.dev.java.net
> >>
> >>
> >>
> >
> >---------------------------------------------------------------------
> >To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> >For additional commands, e-mail: meframework-help@cqme.dev.java.net
> >
> >
> >
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Dmitri Trounine

Hi Vladimir,

I'am marking you letter with listed issues as 'important' and will file
all bugs after commit. Also Alexey has tested the preview of my updates
and discovered similar issues. He is going to write a short report,
together we will not let the bugs pass.

I've seen your mails about other updates, thanks a lot! I'm going to
commit them soon.

Thanks,
Dmitri.

Vladimir Sizikov wrote:

>Hi Dmitri,
>
>On Mon, Mar 19, 2007 at 07:07:20PM +0300, Dmitri Trounine wrote:
>
>
>>Thanks for review. I'll add Javadoc info to my code and will commit.
>>Then I'll file issues that you have mentioned and we will proceed with
>>new fixes and reviews (including low performance problem). Ok?
>>
>>
>
>Sounds good. Please make sure you filed all the appropriate bugs in
>the Issue Tracker, I will definitely forget them if no bugs are in the
>database, too many things happen simultaneously, and it's hard to keep
>all the issues in my little head.
>
>Btw, just to make sure we are in sync, I reviewed all your proposed
>bugfixes and responded in separete threads. Please let me know if I
>missed anything.
>
>Thanks,
> --Vladimir
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Vladimir Sizikov

Dmitri,

On Mon, Mar 19, 2007 at 07:22:45PM +0300, Dmitri Trounine wrote:
> I'am marking you letter with listed issues as 'important' and will file
> all bugs after commit. Also Alexey has tested the preview of my updates
> and discovered similar issues. He is going to write a short report,
> together we will not let the bugs pass.

Yes, I find this practice of preliminary testing by QA folks very
useful, at least for the big updates like yours.

> I've seen your mails about other updates, thanks a lot! I'm going to
> commit them soon.

Great! I'll wait for your commits and then ask the Release Engineering
to make out-of-cycle promotion.

Thanks,
--Vladimir

> Vladimir Sizikov wrote:
>
> >Hi Dmitri,
> >
> >On Mon, Mar 19, 2007 at 07:07:20PM +0300, Dmitri Trounine wrote:
> >
> >
> >>Thanks for review. I'll add Javadoc info to my code and will commit.
> >>Then I'll file issues that you have mentioned and we will proceed with
> >>new fixes and reviews (including low performance problem). Ok?
> >>
> >>
> >
> >Sounds good. Please make sure you filed all the appropriate bugs in
> >the Issue Tracker, I will definitely forget them if no bugs are in the
> >database, too many things happen simultaneously, and it's hard to keep
> >all the issues in my little head.
> >
> >Btw, just to make sure we are in sync, I reviewed all your proposed
> >bugfixes and responded in separete threads. Please let me know if I
> >missed anything.
> >
> >Thanks,
> > --Vladimir
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Dmitri Trounine

Hi Vladimir,

Thanks for having took a look at my updates! You are rights, my updates
may be better organized, and currently I'm thinkink hard about how to
separate them and make them independent. And as you have noticed, it's
not always easy or even possible.

My first update (in the suite of not yet reviewed updates) was large
update adding support of distributed tests in 'Test Export' mode. This
update also includes the fix for issue #38 "export dir is overwritten"
(my bad, I had to fix it in separate branch...).

Then, I continued with several updates with bugfixes for issues 60, 57
and "Improved export of sources" (need to create issue for this). At the
beginning I tried to do these updates in separate branch, but I realized
that they depend much on previous large update. It was possible to do
these updates separately, but in this case they would need to be
reworked much in order to merge with previous large update.

So, all my updates are now in
branches/users/dtrounine/testExport/mergedDistributedTests.

I propose to review my updates sequentially, starting from the 'export
of distributed tests' update. Each update is bounded by two revision
numbes, so it's possible to see the diff corresponding exactly to each
update and see the source after each update. I can generate such diffs...

Will it work for you?

In any way, I'm going to spend some time today on organizing my updates.

Thanks,
Dmitri.

Vladimir Sizikov wrote:

>Hi Dmitri, team,
>
>Let's devote this thread to the code reviews for the latest updates
>for "Test Export" feature.
>
>Let's discuss first what exactly should be reviewed, there were lots
>of changes, in various branches, and even multiple bugfixes in the
>single branches, with some merges between the branches.
>
>Frankly, I'm lost a little bit.
>
>Dmitri, you've done A LOT :) Let's sort it out and make it easier to
>review.
>
>Typically, I'd suggest to create a separate branch for every bugfix,
>and make the separated bugfixes as independent from each other as
>possible. I understand that this is not always possible.
>
>But it is pretty hard for everybody, to have multiple bugfixes in the
>single branch.
>
>At the moment, my understanding is that there are 4 different issues
>that were (are being?) fixed:
>
>- Export of distributed tests
>- Issue #60: In Test Export mode, not all copied sources are listed.
>- Issue #57: Test Export creates and uses src.map file
>- Improved export of test sources (or is this part of ISSUE #60?)
>- Issue #38: Export directory is overwritten without warnings
>
>Am I missing something, were there any other bugfixes?
>
>I took a look at the some changes in the Dmitri's branch, but there
>are so many of them, and I'm afraid that I could spend quite some
>time trying to review something that's not needed to be reviewed...
>
>Dmitri, would it be possible for you to create independent branches
>for every bugfix, with clear bug id associated with every change, with
>bugs' evaluations updated with information about the bugfix (what was
>done and why), and post the relevant info into this thread?
>
>I'm really sorry, that's a bit of extra work for you. Let me know if
>the request is not feasible.
>
>From my side, I promise to devote one full day reviewing all your
>changes, once you provide the list of bugfixes, with updated bug
>reports, with separated branches.
>
>Deal? :)
>
>Given the amount of the code changes, I think that there should be
>some unit tests provided. Some time ago we discussed unit tests policy
>and my understanding is that we should strive to provide unit tests
>for every new feature, especially for such big changes.
>
>(Oh, I just noticed that you do provide some unit test, for new parser
>functionality. But do we need to provide some more unit tests for all
>other changes?).
>
>Also, please note that some areas must be reviewed by Stan, and we
>cannot commit them without his approval (packages com.sun.tck.cldc and
>com.sun.cldc). In your case, TestBundler must definitely be reviewed
>by Stan. The sooner you submit such CLDC-related changes for his
>review, the sooner you get the response.. :)
>
>Thanks,
> --Vladimir
>
>P.S. With big updates, it's always hard to properly review them, so it
>would be great to find a balance somehow, and provide changes in more
>iterative process, with a list of smaller fixes rather then with one
>huge change. But again, it's not always easy to figure this out.
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net