Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accept arguments to aiida_profile #3481

Closed
wants to merge 3 commits into from

Conversation

espenfl
Copy link
Contributor

@espenfl espenfl commented Oct 30, 2019

An initial solution to how to pass arguments to the recently introduced aiida_profile. The problem with this approach is that we on the plugin side relies on fixtures, which typically use the aiida_profile fixture as well. Since there is no way, using this approach to pass arguments from one fixture to another (say if we want to set pg_ctl in the pgtest argument to aiida_profile).

We would thus severely limit the usability of fixtures on the plugin side.

@espenfl
Copy link
Contributor Author

espenfl commented Oct 30, 2019

In the second commit a factory function have been used, which makes it possible to pass arguments. However, when passing e.g. {'pgtest': {'pg_ctl': '/usr/pgsql-9.6/bin/pg_ctl'} into test_arguments of test_manager we get the errors reported in #3482. However, that is a separate issue.

Is this something we can run with? I have heard rumors that factory functions obfuscate the test printouts in case there is something amiss, but have not yet seen this myself (or cared to much about it).

@ltalirz
Copy link
Member

ltalirz commented Oct 30, 2019

It seems @greschd solved this type of problem by using a configuration file for tests instead.

If you ask me, I would lean towards the configuration file approach (see relevant links here) - adding arguments makes both the definition and the usage of these fixtures rather complicated.

Perhaps @greschd would be willing to come up with a minimal PR that adds support for such a configuration file?

@ltalirz
Copy link
Member

ltalirz commented Oct 30, 2019

Anyhow, it's not clear to me what the best solution is - perhaps it is not possible to keep the fixtures simple and configurable enough for everybody at the same time.

Another approach is to say:

  • we keep the fixtures that ship directly with aiida-core simple, so that they are easy to understand and use for plugin developers who are just starting out
  • we make sure that the test_manager context manager is highly configurable

and suggest to developers who have particular needs write their own fixtures using the manager (after all, the fixtures themselves are very little code).

Perhaps @chrisjsewell or @sphuber also have some ideas.

@espenfl
Copy link
Contributor Author

espenfl commented Oct 30, 2019

Happy to take a look at your solution @greschd for instance for the pgtest parsing.

With respect to complexity I do not think that is a problem as long as we support aiida_profile(). Passing whatever arguments down (well, basically right now it is just pgtest in addition to backend and profile_name as far as I can see) does not increase complexity.

Having said that, I am happy with other solutions and this was just a suggestion following the lines we had before and the newly introduced concept. I think in principle we should avoid factory functions if we can, but if this is the only solution to offer parameters to pgtest, so be it.

@greschd
Copy link
Member

greschd commented Oct 30, 2019

@espenfl In your use case, would different developers use different pg_ctl executables? In that case it would probably make sense to have some config file approach. But we would still need to decide at which point in the code to inject the config values - this might work nicely with @ltalirz's proposal to have a configurable base with sugar on top for "casual" use. The top-layer fixtures could use the configuration, with little extra effort required to go around that.

This particular use case seems somewhat different from the things I proposed in #3473 (computer & code setup) in that it determines how the DB itself is set up, whereas the other things happen after that initialization step. That doesn't necessarily mean that we have to treat it in a different way, though.

@espenfl
Copy link
Contributor Author

espenfl commented Oct 30, 2019

@greschd Yes and no. One could also envision testing on different Postgresql versions, but let us leave that option for now. Also such a test belongs to aiida_core and not the plugins I believe, so fixture nesting would be less of a problem.

Also, opening up for a config file is certainly maybe the best option. I totally missed that this was considered. Sure, let us just open for a pgtest section there and then we can parse whatever pgtest eats in the json, following the formatting expected from pgtest.

Closing this now.

@espenfl espenfl closed this Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants