Skip to content

Comments

replace live server calls in tests/test_tasks/test_task.py#1683

Open
omkar-334 wants to merge 1 commit intoopenml:mainfrom
omkar-334:test-task
Open

replace live server calls in tests/test_tasks/test_task.py#1683
omkar-334 wants to merge 1 commit intoopenml:mainfrom
omkar-334:test-task

Conversation

@omkar-334
Copy link

Metadata
Reference Issue: Fixes #1649
New Tests Added: No
Documentation Updated: NA
Change Log Entry: Replace live server calls with mocks in test_task.py

@omkar-334 omkar-334 marked this pull request as ready for review February 21, 2026 14:54
Copilot AI review requested due to automatic review settings February 21, 2026 14:54
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR attempts to replace live server calls with mocks in tests/test_tasks/test_task.py as part of a larger effort to eliminate flaky tests that depend on external servers (Issue #1649). The changes mock the task upload functionality by patching requests.Session.post and providing a mock XML response for successful task creation.

Changes:

  • Added mocking for test_upload_task to avoid live server calls during task publishing
  • Removed @pytest.mark.test_server() decorators from both test methods
  • Added mock response XML file for task upload with ID 99999

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File Description
tests/test_tasks/test_task.py Added mocks for task upload test and removed test server markers; however, mocking is incomplete as helper methods and download test still make server calls
tests/files/mock_responses/tasks/task_upload.xml New mock XML response file for successful task upload with placeholder task ID

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

super().setUp()

@pytest.mark.test_server()
def test_download_task(self):
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

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

The test_download_task method still makes a live server call through get_task but no longer has the @pytest.mark.test_server decorator. This test needs to be mocked to avoid hitting the live server. Consider mocking requests.Session.get to provide a mock task XML response.

Suggested change
def test_download_task(self):
@mock.patch.object(requests.Session, "get")
def test_download_task(self, mock_get):
tasks_dir = self.static_cache_dir / "mock_responses" / "tasks"
mock_get.return_value = create_request_response(
status_code=200, content_filepath=tasks_dir / "task.xml"
)

Copilot uses AI. Check for mistakes.
Comment on lines +53 to +54
with mock.patch.object(OpenMLTaskTest, "_get_compatible_rand_dataset", return_value=[20]), \
mock.patch.object(OpenMLTaskTest, "_get_random_feature", return_value="class"):
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

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

The mocked methods _get_compatible_rand_dataset and _get_random_feature bypass the original logic, but the actual methods at lines 87-122 still call list_datasets and get_dataset, which make server calls. If any subclass or test directly calls these helper methods, they will hit the live server. Consider adding mocks for requests.Session.get to mock the responses from list_datasets and get_dataset to make this test truly isolated.

Copilot uses AI. Check for mistakes.
Comment on lines +56 to 85
for i in range(100):
try:
dataset_id = compatible_datasets[i % len(compatible_datasets)]
# TODO consider implementing on the diff task types.
task = create_task(
task_type=self.task_type,
dataset_id=dataset_id,
target_name=self._get_random_feature(dataset_id),
estimation_procedure_id=self.estimation_procedure,
)

task.publish()
TestBase._mark_entity_for_removal("task", task.id)
TestBase.logger.info(
f"collected from {__file__.split('/')[-1]}: {task.id}",
)
# success
break
except OpenMLServerException as e:
# Error code for 'task already exists'
# Should be 533 according to the docs
# (# https://www.openml.org/api_docs#!/task/post_task)
if e.code == 614:
continue
else:
raise e
else:
raise ValueError(
f"Could not create a valid task for task type ID {self.task_type}",
)
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

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

With mocking in place, the 100-iteration loop is no longer necessary because the server errors (code 614) won't occur with a mocked response. The test could be simplified to execute only once, which would make it faster and clearer. The retry logic was designed to handle race conditions on the live server, which don't exist when using mocks.

Copilot uses AI. Check for mistakes.
Comment on lines +43 to 51
mock_post.side_effect = [
create_request_response(
status_code=200, content_filepath=tasks_dir / "task_upload.xml"
),
]

# We don't know if the task in question already exists, so we try a few times. Checking
# beforehand would not be an option because a concurrent unit test could potentially
# create the same task and make this unit test fail (i.e. getting a dataset and creating
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

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

The mock_post.side_effect provides only one response, but the loop can iterate up to 100 times (lines 56-85), each calling task.publish() which makes a POST request. After the first iteration, subsequent calls to mock_post will raise a StopIteration error because side_effect has been exhausted. Either simplify the test to run once (recommended since mocking eliminates the need for retries), or provide 100 mock responses if the retry logic must be tested.

Suggested change
mock_post.side_effect = [
create_request_response(
status_code=200, content_filepath=tasks_dir / "task_upload.xml"
),
]
# We don't know if the task in question already exists, so we try a few times. Checking
# beforehand would not be an option because a concurrent unit test could potentially
# create the same task and make this unit test fail (i.e. getting a dataset and creating
mock_post.return_value = create_request_response(
status_code=200, content_filepath=tasks_dir / "task_upload.xml"
)
# We don't know if the task in question already exists, so we try a few times. Checking
# beforehand would not be an option because a concurrent unit test could potentially
# create the same task and make this unit test fail (i.e. getting a dataset and creating
# beforehand would not be an option because a concurrent unit test could potentially
# create the same task and make this unit test fail (i.e. getting a dataset and creating

Copilot uses AI. Check for mistakes.
Comment on lines 49 to 52
# We don't know if the task in question already exists, so we try a few times. Checking
# beforehand would not be an option because a concurrent unit test could potentially
# create the same task and make this unit test fail (i.e. getting a dataset and creating
# a task for it is not atomic).
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

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

The comment mentions "Checking beforehand would not be an option because a concurrent unit test could potentially create the same task" which refers to race conditions on the live server. With mocking, there are no concurrent tests accessing a shared server state, so this comment is no longer accurate and should be updated or removed to reflect that the test now uses mocks.

Suggested change
# We don't know if the task in question already exists, so we try a few times. Checking
# beforehand would not be an option because a concurrent unit test could potentially
# create the same task and make this unit test fail (i.e. getting a dataset and creating
# a task for it is not atomic).
# We don't know if the task in question already exists on the server, so we try creating
# it multiple times and handle the "task already exists" error (code 614) as it would
# occur in real-world usage. This test simulates that behavior using mocked responses.

Copilot uses AI. Check for mistakes.
Comment on lines +74 to +81
except OpenMLServerException as e:
# Error code for 'task already exists'
# Should be 533 according to the docs
# (# https://www.openml.org/api_docs#!/task/post_task)
if e.code == 614:
continue
else:
raise e
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

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

The exception handling for OpenMLServerException with error code 614 ("task already exists") is designed for live server interactions where tasks might already exist. With mocking, the mock response is predetermined and won't raise this exception unless explicitly configured to do so. This exception handling is now dead code and can be removed when the test is simplified to run once with the mock.

Copilot uses AI. Check for mistakes.
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.

[MNT] Replace Live Server Calls with Mocks in Unit Tests

1 participant