Empty strings in DIDSet transaction #717
Conversation
Co-authored-by: Mayukha Vadari <[email protected]>
Co-authored-by: Mayukha Vadari <[email protected]>
| with self.assertRaises(XRPLModelException): | ||
| DIDSet(account=_ACCOUNT, data="", did_document="", uri="") | ||
|
|
||
| def test_remove_multiple_fields(self): |
There was a problem hiding this comment.
IMO each transaction should be a separate test. Easier to run.
There was a problem hiding this comment.
I'd like to remove consecutive fields in the DID object, in order to demonstrate that users cannot leave behind an empty DID object.
I need to perform consecutive removals on the same object, it cannot be demonstrated with a new DID ledger object
There was a problem hiding this comment.
That's not a library test, that's a rippled test. And even if it were a library test, it should happen in an integration test, because it won't happen in a unit test (the unit test doesn't submit transactions and therefore doesn't create/modify/delete ledger objects). All you're doing here is creating the models, not submitting anything.
There was a problem hiding this comment.
okay, I've created an integration test for the same
…o test in unit tests format the docs for DIDSet model data fields
|
I have reverted back to an older commit. I removed the integration test and restored the unit test. Let me know if this looks ok |
| self.assertTrue(tx.is_valid()) | ||
|
|
||
| # remove the data field from the above DID object | ||
| tx = DIDSet( |
There was a problem hiding this comment.
Do you still want a transaction for each field?
There was a problem hiding this comment.
I'm only removing the data field. I'm not using any other transaction
There was a problem hiding this comment.
You originally also removed the other fields.
There was a problem hiding this comment.
yes, that's correct. but now, I'm only empty-ing the data field
There was a problem hiding this comment.
Yes, should there be tests for emptying each field?
There was a problem hiding this comment.
can you tell me the name of the unit test that you are objecting to?
test_empty_data_field -- a unit test that empties only the data field. all the other fields contain valid values.
test_create_did_object_all_empty_fields -- a unit test that validates the initial input for the DIDSet transaction. This verifies that all the input fields are not empty.
You are correct that rippled has already tested this functionality. Which one of these warrants a change?
High Level Overview of Change
Fix #716
Context of Change
Fields within a DID object can only be deleted by setting an empty string. However, the regex in the
DIDSetmodel do not account for empty strings.Type of Change
Did you update CHANGELOG.md?
Test Plan
Unit tests have been added to this effect