Skip to main content

[Fwd: Re: Please review: fix for issue #100]

1 reply [Last post]

Reply viewing options

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

Hi Dmitri,

I'm OK with changes. Just one comment - I think commented code should be
deleted. Then you can commit.


Dmitri Trounine > wrote:
> Hi Alexander,
> I've implemented changes that we discussed in previous messages. Please
> review:
> Thanks,
> Dmitri.
> Dmitri Trounine > wrote:
>> Alexander Alexeev wrote:
>>> As I can see you adds only sources from src/share/classes dirs. How
>>> test's sources are added? Or are they always provided through
>>> "source" tag of test's description?
>> I should explain this stuff with sources:
>> - Our aim is to provide a Java source file for each class file
>> contained in the exported JAR file (except the cases like A$B.class
>> and similar).
>> - I divide all classes into four groups:
>> 1. Classes for agent and client. They come in JAR files from ME
>> Framework.
>> 2. Classes for test suite specific libraries. They come in JAR
>> files from test suites.
>> 3. Test classes. They come from separate class files listed in
>> testClasses.lst.
>> 4. Test library classes. They also come from separate class
>> files and are also listed in testClasses.lst.
>> - How the classes are added to test JAR bundles?
>> - Classes 1 and 2 are added to test JAR bundles via
>> JarBuilder.addJarFileContent() method.
>> - Classes 3 and 4 are added to test JAR bundles via
>> JarBuilder.addByteArray(s) methods invoked from within
>> JarBuilder.addTest() method.
>> - Where are located the source?
>> - In case 1, sources are in the ME Framework src/share/classes
>> directory.
>> - In case 2, sources are located in the test suite
>> src/share/classes directory.
>> - In case 3, sources are located near the test description in
>> the tree under the test suite 'tests' directory. These sources are
>> listed in test descriptions.
>> - In case 4, sources are located in the test suite
>> src/share/classes/ directory.
>> How these cases are handled:
>> - Case 1: JarBuilder.addJarFileContent() is overridden in
>> MidExportBuilder. New implementation reads the content of added JAR
>> file and looks for sources in ME Framework 'src/share/classes' directory.
>> - Case 2: Same as in case 1, but the test suite
>> 'src/share/classes' directory is scanned.
>> - Case 3: Easy. ExportBundler reads the test descriptions and
>> just copies the sources.
>> - Case 4: JarBuilder.addByteArray(s) methods are overridden in
>> MidExportBuilder so that they looks for sources in the test suite
>> 'src/share/classes' directory.
>> The issue #100 is about cases 2 and 4:
>> - MidExportBuilder.getSourceForClass() did only scan the ME
>> Framework 'src/share/classes' directory. So, the case 2 was not
>> properly handled if ME Framework and the test suite
>> 'src/share/classes' directories are distinct. This is what you noticed
>> in your previous comments.
>> - Case 4 was not implemented at all.
>> In order to fix issue #100 we have to fix handling of case 2 and
>> add handling of case 4.
>>>>> The next comment about the name of private method add(name,
>>>>> content). This name
>>>>> is a bit of confusing. It doesn't create "name" entry with
>>>>> specified "content".
>>>>> It just tries to find source corresponding to the "name". So may be
>>>>> call it
>>>>> something like "tryToAddSource" and remove second argument.
>>>> I renamed it to registerClass(...) just because there is another
>>>> method registerTestDescription(...) which does the same: collects
>>>> information. I do not like name tryToAddSources because it's too
>>>> specific. One little change in code in future will made such name
>>>> meaningless.
>>> It's really too specific and this is intentional. The method should have
>>> *self-explained* name. Now you have the method with very common name
>>> and with
>>> irrelevant set of arguments. You even had to provide comments to the
>>> code to
>>> explain what the code do. I'm really prefer the code that could be easy
>>> understood without reading comments. I'm not saying comments are bad.
>>> No, comments are *good* but the code that requires comments is bad
>>> very often.
>> Agreed. Changing the name of this methof to tryToAddSource.
>>>>> Yet another comment. Why you are overriding addByteArray() and
>>>>> addByteArrays()?
>>>>> As I understand these methods is intended to create new entries in
>>>>> the jar file
>>>>> with the specified content and the names may not correspond to real
>>>>> file names
>>>>> in the file system. I think more safe to retrieve names using
>>>>> getTestFiles(). Or
>>>>> I've missed something and addByteArray(s)() is used to add some
>>>>> class files are
>>>>> not listed in the getTestFiles()?
>>>> Yeah, may be it's not an absolutely neccessary to use addXXX()
>>>> methods for this purpose. It's just my way to think about
>>>> JarBuilder. Invocation of addXXX() methods is an event that we
>>>> should handle. When one of these method is invoked, we think:
>>>> something is about to be added to the JAR file and we should take
>>>> care of this. In such way we are able to handle all and won't miss
>>>> anything.
>>>> About filenames... When a class is added, it should be in a class
>>>> file. And the name of this class file should reflect the pacakage
>>>> name of the class. The things work like that in Java. So, if it's
>>>> not a class file, it's a really strange thing and the Test Suite
>>>> developers should take care of this by extending MidExportBuilder
>>>> and overriding addXXX method so that their content is being handled
>>>> in proper way. And if it's a class file we just look for it's Java
>>>> source file.
>>> Yes, in this particular case it will work but it looks like a hack.
>>> But why to
>>> use a hack if we can provide more clear solution?
>> Since the test descriptions don't list *all* sources for test, all
>> that follows from that is hack! :)
>>> getTestFiles() should provide
>>> all the classes are needed for test execution on a device side. I
>>> think it is
>>> exactly that you need.
>> Ok, let change it as you proposed. Indeed, getTestFiles() should do
>> the same thing. Note: in my terms it's about the case 4.
>>> And I've started to view ExportBundler. Seems testSourcesMap variable
>>> and
>>> getSources() method are useless since method put() for the map is
>>> called only in
>>> two places: in getSources() where empty list is added and in
>>> registerTestSource() which is never used. Or I've missed something?

To unsubscribe, e-mail:
For additional commands, e-mail: