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

raise exception on failed delete from database #1375

Merged
merged 2 commits into from
Mar 19, 2024

Conversation

XzzX
Copy link
Contributor

@XzzX XzzX commented Mar 15, 2024

No description provided.

@XzzX XzzX requested a review from jan-janssen March 15, 2024 09:56
@XzzX
Copy link
Contributor Author

XzzX commented Mar 15, 2024

The exception is also raised if the job does not exist. Do we want to handle this separately?

@XzzX XzzX force-pushed the exception_on_failed_delete branch from 14057f2 to ad0a5df Compare March 15, 2024 10:02
@jan-janssen
Copy link
Member

@XzzX Can you add a unit test for this functionality? We already have a test for deleting jobs:
https://github.com/pyiron/pyiron_base/blob/main/tests/database/test_database_access.py#L184
So it would be great to add a test to delete a job that does not exist.

@jan-janssen jan-janssen marked this pull request as draft March 15, 2024 10:13
@XzzX
Copy link
Contributor Author

XzzX commented Mar 15, 2024

@XzzX Can you add a unit test for this functionality? We already have a test for deleting jobs: https://github.com/pyiron/pyiron_base/blob/main/tests/database/test_database_access.py#L184 So it would be great to add a test to delete a job that does not exist.

You are already testing this, aren't you? There is a normal delete and a raise exception delete... I am confused. In my notebook there is no exception fired.

@jan-janssen
Copy link
Member

@XzzX the current test only covers the regular delete and trying to submit the job ID as list which is not supported. There is currently no test to delete a job which no longer exists:

def test_delete_item(self):
        """
        Tests delete_item function
        Returns:
        """
        par_dict = self.add_items("BO")
        key = par_dict["id"]
        self.database.delete_item(key)
        self.assertRaises(
            Exception, self.database.delete_item, [key]
        )  # use only str or int
        # self.assertRaises(Exception, self.database.get_item_by_id, key)  # ensure item does not exist anymore

@XzzX
Copy link
Contributor Author

XzzX commented Mar 15, 2024

@XzzX the current test only covers the regular delete and trying to submit the job ID as list which is not supported. There is currently no test to delete a job which no longer exists:

def test_delete_item(self):
        """
        Tests delete_item function
        Returns:
        """
        par_dict = self.add_items("BO")
        key = par_dict["id"]
        self.database.delete_item(key)
        self.assertRaises(
            Exception, self.database.delete_item, [key]
        )  # use only str or int
        # self.assertRaises(Exception, self.database.get_item_by_id, key)  # ensure item does not exist anymore

Aaaah, you are checking for a type error. Now also the comment makes sense. Ok, got it.

@jan-janssen jan-janssen marked this pull request as ready for review March 15, 2024 11:42
Copy link
Member

@jan-janssen jan-janssen left a comment

Choose a reason for hiding this comment

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

Looks good to me

@jan-janssen jan-janssen merged commit b749abb into pyiron:main Mar 19, 2024
22 of 23 checks passed
@XzzX XzzX deleted the exception_on_failed_delete branch March 20, 2024 12:03
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.

2 participants