-
-
Notifications
You must be signed in to change notification settings - Fork 213
Feat: replaced hardcoded test server admin key with env variable #1568
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
base: main
Are you sure you want to change the base?
Changes from all commits
808ab63
f183d11
7a94293
325c328
4251321
455dad2
4ad4498
6acd640
7e1319e
e6aff05
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -96,6 +96,20 @@ To test your new contribution, add [unit tests](https://github.com/openml/openml | |||||||||||
| * Please ensure that the example is run on the test server by beginning with the call to `openml.config.start_using_configuration_for_example()`, which is done by default for tests derived from `TestBase`. | ||||||||||||
| * Add the `@pytest.mark.sklearn` marker to your unit tests if they have a dependency on scikit-learn. | ||||||||||||
|
|
||||||||||||
| #### Running Tests That Require Admin Privileges | ||||||||||||
|
|
||||||||||||
| Some tests require admin privileges on the test server and will be automatically skipped unless you provide an admin API key. For regular contributors, the tests will skip gracefully. For core contributors who need to run these tests locally: | ||||||||||||
|
|
||||||||||||
| **Set up the key** by exporting the variable: | ||||||||||||
| run this in the terminal before running the tests: | ||||||||||||
|
Comment on lines
+101
to
+104
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
|
|
||||||||||||
| ```bash | ||||||||||||
| # For windows | ||||||||||||
| $env:OPENML_TEST_SERVER_ADMIN_KEY = "admin-key" | ||||||||||||
| # For linux/mac | ||||||||||||
| export OPENML_TEST_SERVER_ADMIN_KEY="admin-key" | ||||||||||||
| ``` | ||||||||||||
|
|
||||||||||||
| ### Pull Request Checklist | ||||||||||||
|
|
||||||||||||
| You can go to the `openml-python` GitHub repository to create the pull request by [comparing the branch](https://github.com/openml/openml-python/compare) from your fork with the `develop` branch of the `openml-python` repository. When creating a pull request, make sure to follow the comments and structured provided by the template on GitHub. | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ | |
|
|
||
| OPENML_CACHE_DIR_ENV_VAR = "OPENML_CACHE_DIR" | ||
| OPENML_SKIP_PARQUET_ENV_VAR = "OPENML_SKIP_PARQUET" | ||
| OPENML_TEST_SERVER_ADMIN_KEY_ENV_VAR = "OPENML_TEST_SERVER_ADMIN_KEY" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in case of using a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes definitely! Will go ahead with the new dep if approved. |
||
| _TEST_SERVER_NORMAL_USER_KEY = "normaluser" | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -106,6 +106,10 @@ def test_setup_with_config(self): | |||||||||
|
|
||||||||||
|
|
||||||||||
| class TestConfigurationForExamples(openml.testing.TestBase): | ||||||||||
| @pytest.mark.skipif( | ||||||||||
| not os.environ.get(openml.config.OPENML_TEST_SERVER_ADMIN_KEY_ENV_VAR), | ||||||||||
| reason="Test requires admin key. Set OPENML_TEST_SERVER_ADMIN_KEY environment variable.", | ||||||||||
| ) | ||||||||||
|
Comment on lines
+109
to
+112
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think we can skip this check and make the test not require the admin key (see other comment I left below). This is what I meant to communicate with https://github.com/openml/openml-python/pull/1568/changes#r2698514445 |
||||||||||
| @pytest.mark.production() | ||||||||||
| def test_switch_to_example_configuration(self): | ||||||||||
| """Verifies the test configuration is loaded properly.""" | ||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. btw this test passes anyways even if you use any random
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, But FAILED tests/test_datasets/test_dataset_functions.py::TestOpenMLDataset::test_data_status - openml.exceptions.OpenMLServerException: https://test.openml.org/api/v1/xml/data/status/update ...
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I think the more reasonable approach here is to update this test to use some other sentinel value.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Just anything that helps verify that |
||||||||||
|
|
||||||||||
Uh oh!
There was an error while loading. Please reload this page.