Correct PID binding tests #144
Conversation
ross-spencer
force-pushed
the
dev/issue-133-bind-pids-failing
branch
from
a3fd7a7
to
794527d
Compare
There was a problem hiding this comment.
@ross-spencer This looks good to me. I focused on some of the version specific changes at the beginning but then realized you're proposing we discuss that in general 👍
Just out of curiosity, is there any written document/plan about how are going to transition off selenium?
| @@ -451,7 +451,7 @@ def ensure_default_processing_config_in_default_state(self): | |||
| ) | |||
| self.set_processing_config_decision( | |||
| decision_label="Select file format identification command (Ingest)", | |||
| choice_value="Use existing data", | |||
| choice_value="No, use existing data", | |||
There was a problem hiding this comment.
Have we decided how to approach version-driven changes like this?
There was a problem hiding this comment.
That's a good point, this definitely lends itself to that conversation. I'll submit something to take care of this for now, and we can open that discussion, possibly in the AM weekly later.
| "premis:object/" | ||
| "premis:objectIdentifier", | ||
| "premis3:object/" | ||
| "premis3:objectIdentifier", |
There was a problem hiding this comment.
I was going to mention this is another version-based change when I kind of remembered a related comment from you on this. Found it in one of the connected issues description! "Additionally, there is a discussion to be had here about the expectation of the AMAUAT tests to pass against different versions of Archivematica where PREMIS 3 wasn't previously used" 👍
There was a problem hiding this comment.
Looking through changes in the suite it seems we already have some code already looking for PREMIS3. I'm going to submit a multi-version fix, but I think this makes me want to err on removing multiple versions moving forward. I suspect we can't already get the whole suite to pass on 1.9. IDK though, I haven't as much experience with AMAUAT.
features/steps/pid_binding_steps.py
Outdated
| context.am_user.mets.assert_empty_dir_documented_identified( | ||
| mets, empty_dir_rel_path | ||
| ) | ||
| assert True |
There was a problem hiding this comment.
Should we document why this has become an empty step?
There was a problem hiding this comment.
Totally a mistake on my part. Reinstated the code! Otherwise yes, would totally have documented this! 😜
features/steps/utils.py
Outdated
| @@ -85,7 +85,13 @@ def get_event_attr(event_type): | |||
| return "{}_event_uuid".format(event_type) | |||
|
|
|||
|
|
|||
| def get_mets_from_scenario(context): | |||
| def get_mets_from_scenario(context, api=False): | |||
| """Retrieve the AIP METS file from the AIP package.""" | |||
There was a problem hiding this comment.
Nitpick (feel free to ignore): this docstring reads strange when compared with the method name (where are we getting the METS from?).
There was a problem hiding this comment.
I think that's a great point. Just modifying that now!
|
@replaceafill I've given this a second go based on your considered thoughts - if you'd like to re-review? I feel like a good way forward might be to approve and merge this work (if it passes approval!) then resolve the discussion around versions. And then complete that work separately, e.g. to slowly remove decision points.
None that I am aware of. I imagined it would be an organic thing that falls out of improvements and code-review. For example, in this feature I've approached this from the perspective that we're not testing the mechanism that downloads the METS explicitly. There are other tests that do that too, and so we should focus on the METS once it is available in the AIP which will make the test more robust, like the black-box features, using the API, and then we've one lest test reliant on Selenium and one test that is much less brittle. But I think we should keep having that discussion if testing the download from the browser mechanism is crucial to any particular test. Or a crucial part of Archivematica that warrants its own test. For other readers, the proposal to remove the need to create version decision points in the AMAUAT tests is here: archivematica/Issues#778 |
features/steps/steps.py
Outdated
| @@ -109,18 +109,18 @@ def step_impl(context): | |||
| "Given that the user has ensured that the default processing config is" | |||
| " in its default state\n" | |||
| 'And the processing config decision "Select file format identification' | |||
| ' command (Transfer)" is set to "Identify using Fido"\n' | |||
| ' command (Transfer)" is set to "Yes"\n' | |||
There was a problem hiding this comment.
Hmm, @replaceafill I just realized that these are version specific too. Will have a look now before review...
ross-spencer
force-pushed
the
dev/issue-133-bind-pids-failing
branch
from
83beda1
to
e8dea3a
Compare
|
Hi @replaceafill, I think this is good to re-review now. I have added two commits since the last CR.
As per this comment we don't need to worry about keeping these tests backwards compatible (thank you for raising those questions as part of your first code-review), so this, once merged, is likely to be the first in a number of related issues that will bring the AMAUAT tests up to the |
|
@ross-spencer New commits LGTM 👍 |
In this commit we make a handful of small changes to make the PID binding tests pass and slowly start moving to making further use of AMClient API calls. In this instance, we enable the download of AIP METS from a Archival Information Package without having to read it via the selenium driver in the Archivematica ingest browser.
ross-spencer
force-pushed
the
dev/issue-133-bind-pids-failing
branch
from
e8dea3a
to
7ee476d
Compare
In this commit we make a handful of small changes to make the PID
binding tests pass and slowly start moving to making further use of
AMClient API calls. In this instance, we enable the download of
AIP METS from a Archival Information Package without having to read
it via the selenium driver in the Archivematica ingest browser.
Connected to archivematica/Issues#133
Connected to archivematica/Issues#777