On Wed, Sep 25, 2019 at 03:16:55PM +0200, Amador Pahim wrote: > On Tue, Sep 24, 2019 at 9:42 PM Willian Rampazzo <wramp...@redhat.com> wrote: > > > > Hello everyone, > > > > I started working on Trello card > > https://trello.com/c/T3SC1sZs/1521-implement-fetch-assets-command-line > > as part of a broader card, > > https://trello.com/c/CKP7YS6G/1481-on-cache-check-for-asset-fetcher > > and I would like to bring my findings to a discussion. > > > > One way to implement that would be to parse the test source looking > > for the fetch_asset call and execute it. In theory, it is a straight > > forward implementation. > > > > I have started working in the parser. After some simple tests, some > > complex situations started to show up. Let me bring an existing > > example, examples/tests/assets.py: > > > > def setUp(self): > > mirrors = ['https://mirrors.peers.community/mirrors/gnu/hello/', > > 'https://mirrors.kernel.org/gnu/hello/', > > 'http://gnu.c3sl.ufpr.br/ftp/', > > 'ftp://ftp.funet.fi/pub/gnu/prep/hello/'] > > hello = 'hello-2.9.tar.gz' > > hello_locations = ["%s/%s" % (loc, hello) for loc in mirrors] > > hello_sig = 'hello-2.9.tar.gz.sig' > > hello_sig_locations = ["%s/%s" % (loc, hello_sig) for loc in > > mirrors] > > self.hello = self.fetch_asset( > > name=hello, > > locations=hello_locations) > > self.hello_sig = self.fetch_asset( > > name=hello_sig, > > asset_hash='f3b9fae20c35740004ae7b8de1301836dab4ac30', > > locations=hello_sig_locations) > > > > When the parser finds the fetch_asset call, it needs to inspect its > > arguments, looking for variables. If any variable is found, it needs > > to walk back the code looking for its assignment, so it is able to > > execute it prior to the fetch_asset execution. If the variables > > consist of other variables, the parser, again, needs to walk back in > > the code looking for those assignments, and so on. > > > > There are countless ways of creating the right side of a variable > > assignment, from a single string assignment to a function that builds > > the content, or conditional assignments. > > > > My initial idea is to cover variables consisting of other variables > > and list comprehension, just like in the example. For now, other > > complex constructions would be out of this implementation. > > > > Any comment, concern, suggestion, is appreciated here as it would help > > to build a more robust code. > > The problem with not covering all the cases is that, well, you're not > covering all the cases :) > And the parsing of code to execute snippets out of it is always a > tricky thing to do. What if your variable is being filled by a > function call that acquires data from an external source that requires > a specific environment... how to reproduce that environment in the > command liner? There are way too many cases were things can go wrong. > > Have you considered adding support for args-from-config-file in the > asset fetcher? Something like: > > asset.Asset(args_from_config='/foo/bar/my_asset_01.yaml') >
That's an interesting approach, in which the upside is the ease of parsing the data, and the downside is that requiring developers writing tests to use an external file. Requiring the "test" to be split in multiple files may be a useful feature to scale a very large test, but requiring it for all cases is not a good thing. We may find out that, out of the viable options, that this turns out to be the best one, but we should keep the cons in mind. Anyway, this actually reminds me of a older idea, on the topic of parameters, in which we'd use the test's data directories to fetch files containing parameters that would be applied by default to tests. But, if we decide on the data format of those files, say YAML or JSON (wether they contain parameters or asset definitions), we'd be limited by the static nature of it. I remember thinking about that problem, and realizing that a compromise may be to also allow for executable files that would dynamic content. So, for test `my.py:My:test_asset`, we would look for in its data sources directories: * my.py/My/test_asset.data/ * my.py/My.data/ * my.py.data/ Within any of the data directories we could have plugins that would either read the asset files and produce parameters for asset.Asset(), or plugins that would execute files and produce the same thing. For instance: * my.py/My/test_asset.data/assets.json: {"name": "hello-2.9.tar.gz", "locations": ["https://mirrors.peers.community/mirrors/gnu/hello/hello-2.9.tar.gz", "https://mirrors.kernel.org/gnu/hello/hello-2.9.tar.gz", "http://gnu.c3sl.ufpr.br/ftp/hello-2.9.tar.gz", "ftp://ftp.funet.fi/pub/gnu/prep/hello/hello-2.9.tar.gz"], "asset_hash": "cb0470b0e8f4f7768338f5c5cfe1688c90fbbc74"} * my.py/My/test_asset.data/assets.py: #!/usr/bin/env python import json name = "hello-2.9.tar.gz" mirrors = ['https://mirrors.peers.community/mirrors/gnu/hello/', 'https://mirrors.kernel.org/gnu/hello/', 'http://gnu.c3sl.ufpr.br/ftp/', 'ftp://ftp.funet.fi/pub/gnu/prep/hello/'] locations = ["%s/%s" % (loc, name) for loc in mirrors] asset = {"name": name, "locations": locations, "asset_hash": "cb0470b0e8f4f7768338f5c5cfe1688c90fbbc74"} print(json.dumps(asset)) The executable option (my.py/My/test_asset.data/assets.py), while it somehow violates the "don't execute *test* code before running the test" principle, it's an opt-in feature, and still wouldn't be executed while listing tests. When checking and downloading assets, though, it would be executed. > Or, instead of a separate config file, I'd also consider some reserved > keys in the parametrization interface (we have some already: > https://avocado-framework.readthedocs.io/en/latest/WritingTests.html?highlight=timeout#setting-a-test-timeout). > This is similar to: https://trello.com/c/WPd4FrIy/1479-add-support-to-specify-assets-in-test-docstring But your suggestion is better in the sense that users could add dynamic content in such a variable, and at the same time would limit or make the the parsing of such content dificult before executing the test IIUC. - Cleber. > That way you would enable the asset fetcher caching tool to read from > the same configuration input and cache things in advance. Anyone > willing to use that feature would have to either use the > args_from_config also in their tests or specify the args accordingly. > > > > > Best regards, > > > > Willian Rampazzo > > Software Engineer > > Red Hat Brazil > > 2717 337F 7E4A 5FDF > > > > > -- > Pahim >