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

Unit tests #4

Merged
merged 13 commits into from
Apr 28, 2021
Merged

Unit tests #4

merged 13 commits into from
Apr 28, 2021

Conversation

volodymyrss
Copy link
Member

No description provided.

@volodymyrss
Copy link
Member Author

this will need oda-hub/dispatcher-app#69

@volodymyrss
Copy link
Member Author

Note that the one more "real" test successfully fails at the moment. I did not attempt understading what a meaningful request would be. But I was amused by the sort of error message it returns, maybe it points to another bug, maybe even in dispatcher.

We also discussed with @burnout87 that similar approach can be adopted for other plugins

@volodymyrss volodymyrss marked this pull request as ready for review April 27, 2021 19:56
@burnout87
Copy link

I think this is the approach to follow, also for the other plugins

README.md Outdated
- copy the conf_file from 'cdci_antares_plugin/config_dir/data_server_conf.yml' and place in given directory
- set the env var `CDCI_ANTARES_PLUGIN_CONF_FILE` to the path of the file conf_file
- edit the in conf_file the two keys:
- copy the `conf_file` from `cdci_antares_plugin/config_dir/data_server_conf.yml' and place in given directory

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: perhaps also 'config' will have to be inserted in the vocab list

README.md Outdated
- edit the in conf_file the two keys:
- copy the `conf_file` from `cdci_antares_plugin/config_dir/data_server_conf.yml' and place in given directory
- set the environment variable `CDCI_ANTARES_PLUGIN_CONF_FILE` to the path of the file conf_file
- edit the in `conf_file` the two keys:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might got confused, what are the two keys to edit?

@dsavchenko
Copy link
Member

Note that the one more "real" test successfully fails at the moment.

Of course, as to process real query it relies on backend.

But I was amused by the sort of error message it returns, maybe it points to another bug, maybe even in dispatcher.

Not in the dispatcher. I even put a comment and then forgot about it

excep=e, # may be referenced before assignment

@volodymyrss
Copy link
Member Author

Note that the one more "real" test successfully fails at the moment.

Of course, as to process real query it relies on backend.

Alright! Could be there is some meaningful dummy-like query?
Or you could install it as a requirement and test it with a another fixture. If it is possible.

If not, the test should be really considered an "integration", in sense that it will be run on a specific instance, deployed for review.
I.e. it will not work with github actions.
In this case, the backend access will be configured on defining the test.
You can test it while setting your own instance.
And we test it after deploying in unige and also regularly.

But I was amused by the sort of error message it returns, maybe it points to another bug, maybe even in dispatcher.

Not in the dispatcher. I even put a comment and then forgot about it

excep=e, # may be referenced before assignment

ok! It's really a kind of feature common in the dispatcher through. So see as you

@dsavchenko
Copy link
Member

Ideologically it will look more like an integration test

without backend it will fail unless query_type='Dummy' (which itself is the desirable behaviour, which could be tested in units)

For query_type='Dummy' it should inherit the proper dummy behaviour from the base classes defined in dispatcher, no need to implement something specific. Am I right?

@@ -10,19 +10,14 @@ ANTARES cdci plugin is distributed under the terms of The MIT License.

Who's responsible?
-------------------
Andrea Tramacere
Denys Savchenko, Andrea Tramacere

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andriineronov should I put here Paris? APC or IPGP or something?

@andriineronov
Copy link

Just skip "CDCI"

@@ -10,19 +10,14 @@ ANTARES cdci plugin is distributed under the terms of The MIT License.

Who's responsible?
-------------------
Andrea Tramacere
Denys Savchenko, Andrea Tramacere

ISDC Data Centre for Astrophysics, Astronomy Department of the University of Geneva, Chemin d'Ecogia 16, CH-1290 Versoix, Switzerland

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andriineronov I am about this line

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove "ISDC Data Centre for Astrophysics," it does not exist

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could really also do these changes in a separate PR, it's not very clear if we are ok with the changes suggested here.

@dsavchenko
Copy link
Member

For query_type='Dummy' it should inherit the proper dummy behaviour from the base classes defined in the dispatcher, no need to implement something specific. Am I right?

I am not fully right. I got: get_dummy_products needs to be implemented in derived class.

So let's stick with what is here as a baseline. Then I will implement this method and use Dummy query in test

@volodymyrss
Copy link
Member Author

For query_type='Dummy' it should inherit the proper dummy behaviour from the base classes defined in the dispatcher, no need to implement something specific. Am I right?

I am not fully right. I got: get_dummy_products needs to be implemented in derived class.

So let's stick with what is here as a baseline. Then I will implement this method and use Dummy query in test

you are very right though that it's good to stick with the baseline and separate other details features to elsewhere!

@volodymyrss volodymyrss merged commit 55e69c9 into master Apr 28, 2021
@volodymyrss volodymyrss linked an issue Apr 28, 2021 that may be closed by this pull request
@dsavchenko dsavchenko deleted the unit-tests branch April 28, 2021 22:28
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.

make unit test, give an example
5 participants