Skip to content

Correct Call of delete_dataset#96

Merged
sokovninn merged 1 commit intomainfrom
fix/delete-dataset
Jul 16, 2025
Merged

Correct Call of delete_dataset#96
sokovninn merged 1 commit intomainfrom
fix/delete-dataset

Conversation

@JSabadin
Copy link
Contributor

Purpose

The delete_dataset() method was missing arguments, so nothing was being deleted.

Specification

None / not applicable

Dependencies & Potential Impact

None / not applicable

Deployment Plan

None / not applicable

Testing & Validation

None / not applicable

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 fixes the test teardown logic by supplying the missing delete_local argument to the delete_dataset call, ensuring datasets are actually removed during cleanup.

  • Updated delete_dataset invocation in tearDown to include the delete_local=True flag.
Comments suppressed due to low confidence (2)

tests/core_tests/unittests/test_converters.py:281

  • Add an explicit assertion after calling delete_dataset(delete_local=True) to verify that the local dataset directory has been removed, so deletion failures are caught in the test.
            dataset.delete_dataset(delete_local=True)

tests/core_tests/unittests/test_converters.py:281

  • [nitpick] Passing a boolean flag can be unclear; consider splitting into separate methods (e.g., delete_local_dataset() and delete_remote_dataset()) or using an enum/config object to make the deletion intent explicit.
            dataset.delete_dataset(delete_local=True)

@github-actions
Copy link

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
2007 1283 64% 0% 🟢

New Files

No new covered files...

Modified Files

No covered modified files...

updated for commit: 1202ea1 by action🐍

@github-actions
Copy link

Test Results

  6 files    6 suites   1h 23m 13s ⏱️
 87 tests  66 ✅  20 💤 1 ❌
522 runs  397 ✅ 124 💤 1 ❌

For more details on these failures, see this check.

Results for commit 1202ea1.

Copy link
Contributor

@sokovninn sokovninn left a comment

Choose a reason for hiding this comment

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

LGTM

@sokovninn sokovninn merged commit 8b0688a into main Jul 16, 2025
8 of 10 checks passed
@sokovninn sokovninn deleted the fix/delete-dataset branch August 24, 2025 23:17
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