Skip to content

publish-directory - Test repeat publish using --force mode#508

Draft
ce10-sanger wants to merge 6 commits intowtsi-npg:develfrom
ce10-sanger:NPG-3524-test-repeat-publish
Draft

publish-directory - Test repeat publish using --force mode#508
ce10-sanger wants to merge 6 commits intowtsi-npg:develfrom
ce10-sanger:NPG-3524-test-repeat-publish

Conversation

@ce10-sanger
Copy link
Contributor

@ce10-sanger ce10-sanger commented Jan 27, 2026

JIRA: NPG-3524

Fix docs. Default publish_directory(force) is False #488

Test repeat publish using --force mode.

Currently --fill mode fails in case of updated file. Captured addressing and switching to this mode in NPG-3758

@mgcam mgcam self-assigned this Jan 28, 2026
@mgcam mgcam self-requested a review January 28, 2026 09:53
for x in DataObject(dest / "c.txt").metadata()
if x.attribute == "dcterms:created"
]
assert len(created_values) == 1
Copy link
Member

Choose a reason for hiding this comment

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

Assertion on line 225 should be moved here. However, I am not sure what is being tested on line 225. Are we testing that the original file creation date does not change when we asked to update the content of the file? Fair enough. We should also test that the content of the file in iRODS has been updated and a new md5 has been assigned iRODS. So we should capture md5 of the original file on disk and md5 of the updated file on disk, make sure that they differ and check that we get correct md5 and optionally get the content of the file from iRODS and compare to the updated file on disk.

# Arrange
src = Path("./tests/data/ultima/minimal").absolute()
src = tmp_path / "minimal"
shutil.copytree("./tests/data/ultima/minimal", src)
Copy link
Member

Choose a reason for hiding this comment

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

Really, test prep should be set up in a fixture (see conftest.py) so that the majority of the code in the tests is testing things, rather than building state.

@m.context(
'When run in place of npg_publish_tree.pl in "Uploading Ultima run to iRODS" SOP from rnd_platforms'
)
@m.it("Should be compatible with npg_publish_tree.pl")
Copy link
Member

Choose a reason for hiding this comment

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

This is pretty broad - what aspects of the behaviour are being picked out? The following test is large and by the end it has overflowed my context window, so I'm not sure what's covered (and I'm familiar with both systems).

Comment on lines +218 to +245
assert [x.attribute for x in DataObject(dest / "b.txt").metadata()] == [
"dcterms:created",
"dcterms:creator",
"md5",
"type",
]

assert [x.attribute for x in DataObject(dest / "c.txt").metadata()] == [
"dcterms:created",
"dcterms:creator",
"md5",
"md5",
"type",
]
assert [
x.value
for x in DataObject(dest / "c.txt").metadata()
if x.attribute == "md5"
] == [
c_txt_original_md5,
c_txt_modified_md5,
], "New md5 attribute should be added, previous preserved to provide history"
assert (
DataObject(dest / "c.txt").read() == "modified"
), "Contents of data object in iRODS should have updated"
assert (
DataObject(dest / "c.txt").checksum() == c_txt_modified_md5
), "Checksum should have updated"
Copy link
Member

Choose a reason for hiding this comment

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

Take care not to recapitulate tests that are already covered in partisan (which is responsible for what you're testing here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cheers for the feedback Keith. Yea I've been wondering about the structure/approach here too - I'll tighten it up so we've got something tighter to discuss around, share context of what I'm trying to do here and then perhaps we can take a look at it together

@ce10-sanger ce10-sanger marked this pull request as draft February 18, 2026 10:06
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