[Avocado-devel] Implement fetch-assets command line

Sun Sep 29 04:39:58 UTC 2019

Dne 26. 09. 19 v 17:19 Cleber Rosa napsal(a):
> On Thu, Sep 26, 2019 at 09:49:06AM +0200, Lukáš Doktor wrote:
>> Hello guys,
>>
>> what a nice discussion. But before you start, what lead you to pick this card? It's one of the nice-to-have-ideas card and we have plenty of well-defined-and-useful cards that also needs attention, so unless you have a real-world usage, I'd probably suggest to focus on something you can directly benefit from. (unless you have other interest in eg. learning something, or other kind of interest).
> 
> Hi Lukáš,
> 
> Rest assured Willian is pretty much on top of the use case that led
> him to work on this (the pointers he gave later help with that).
> 
> We should question the technical merits and how some work fits into
> the architecture (which you also did later here).  But trying to
> manage anyone's time and effort, even if you think it's in project's
> best interest, is out of place.
> 

I have not tried that. I wanted to understand Willian's use-case so we can actually talk about the technical stuff. You already knew why he picked it, but I didn't get it from the email discussion. Having a use-case is one of the most important things while working on something...

>>
>> Dne 25. 09. 19 v 23:39 Cleber Rosa napsal(a):
>>> On Wed, Sep 25, 2019 at 06:01:22PM -0300, Willian Rampazzo wrote:
>>>> After going thru the suggestions, and thinking about it, I came up
>>>> with the following starting point:
>>>>
>>>> 1 - Support a limited test source code parsing. This feature brings a
>>>> simple solution to those tests already using strings into their
>>>> fetch_asset parameters, as stated by Cleber.
>>>>
>>>
>>> Keep in mind that I'm assuming this is a viable option without
>>> attempting to do it myself.  I hope it's viable, because it makes
>>> "simple things easy" from the test writer's side.
>>>
>>
>> Yes, this makes sense to me and would be useful even for some human-asset interface (list assets, list cached assets, see details of cached assets...). Btw cache-management interface is IMO higher in the priority than actually fetching the assets without executing tests (but it's just my priority list, don't take this as an official statement).
>>
> 
> I can't see a way to separate what I think you call "cache-management
> interface" work without considering that the assets we want to caches
> are defined in tests.
> 

Well the cache-management can use the knowledge generated by fetch calls of the test, therefore the static analysis won't be necessary first. Again, yes, it'd be useful to also allow fetching of all assets beforehand, but having a command that tells me what test downloaded which asset, when it was used the last time and perhaps even check whether it's still up-to-date, see the test-names of the assets as well as paths, perhaps even easier way to download them... These are more useful to me, but I definitely understand someone has different needs, which is again why I asked about the use case.

>>>> 2 - Add an option to use assets.json, as stated by Amador. This file
>>>> would sit in the test data folder and list all assets that are
>>>> necessary for the test. As a list of assets, I thought about a new
>>>> method, fetch_assets, as a wrapper that load all assets from the file
>>>> and call fetch_asset for each one.
>>>>
>>>
>>> Sounds good.  Also consider the need for dynamic asset definition (the
>>> idea about executable scripts that produces JSON).
>>>
>>
>> Hmm, this is tricky. I don't like much having "reserved" file names as people might get unwanted clashes. Yes, it should be fairly harmless with a json file (either it matches or is skipped) but it's a semi-hidden feature. We also have the https://trello.com/c/WPd4FrIy/1479-add-support-to-specify-assets-in-test-docstring that sounds more explicit to me. Another alternative I'd prefer is a `Test` class variable, which can be obtained by a static analysis, or even `Test` variable initialized on `__init__`, which could be executed by the asset fetcher interface (when asked for, because it might eventually execute some nasty code). Having a pre-defined file names in datadir locations is the last interface I'd like to see in Avocado (out of these).
> 
> Behavior "by convention" is pretty much common in the software
> industry, and I don't think it's seen as a bad practice.  Reserved
> words (or file names) are also pretty common, and in fact Avocado
> already does that in the "output check record" feature.
> 
> If you've followed the entire dicussion so far, you've probably
> understood the pros and cons of the approaches discussed.  The
> docstring and class variable with static analysis is limited to
> static content and won't cut it for some types of tests.
> 

Sure, I never said it's a wrong thing, I just mentioned we should consider other ways first. Btw re-reading the discussion again I misunderstood Amador's usage. He did not use "by convention" approach, but string-defined file name, which should be easy to parse with static analysis as well as scalable enough for the users. So that approach is actually not that bad (on the other hand people might need to copy the same file to multiple ".data" locations, how about allowing repo ".data" files as the last level? Fetch assets might then use filters to re-use defined assets).

>>
>> As before, if you have some real-world use case, please share it as it can change everything.
>>
>>
>> class variable
>> --------------
>>
>>     class MyTest(avocado.test):
>>         assets = {"foo": ["http://aaa"], "bar": {"location": "http://fdsa", hash="..."}}
>>
> 
> Analyzing this example staticaly should be easy.  But if, say,
> "location" is code that needs to be evaluated, it just won't work
> without opening a major can of worms.
> 

As usually, bad code can be written, on the other hand static analysis can supple many things at this. People are creative, but "imp" can do pretty well in most cases and for creative people we can have "dangerous" variant that would import the test body.

>> initialized variable
>> --------------------
>>
>>     class MyTest(avocado.test):
>>         def __init__(self, ...):
>>             self.assets = {}
>>             for asset in something:
>>                  self.assets[asset] = get_asset_url(asset)
>>
> 
> No, we want to have knowledge about the assets without executing
> tests.  This won't work.
> 

Again, for creative people using loops to define their assets I can see a "dangerous" variant. I haven't proposed to execute the test, but only to initialize it and only when the user asks for it. As before, it depends on the use case. If the other solutions are sufficient we don't need to discuss the "dangerous" paths.

To summarize my thoughts, I like the basic "fetch_assets" analysis using a regexp or whatever does the job for simple strings, maybe even a list of locations in the future. If we had that I'd have suggested to all users to simply write the asset calls to comply. After re-reading I find Amador's assets-from-files useful for advanced use-cases and should be again easy to parse. The "dangerous" parser might be useful as it should not surprise people, but they already got used-to about test-discovery, so perhaps it's not that important.

Regards,
Lukáš

> - Cleber.
> 
>> ---
>>
>> Regards,
>> Lukáš
>>
> 
> 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/avocado-devel/attachments/20190929/e35d70d3/attachment.sig>