Report a missing test-data path as None rather than the error body

[afgane]

Problem

test_data_path() claims to return a path, and its own callers already treat
that as Optional[str] — test_data_download() assigns the result and checks
it on the very next line
(interactor.py#L583-L585):

local_path = self.test_data_path(tool_id, filename, tool_version=tool_version)

if result is None and (local_path is None or not os.path.exists(local_path)):
    local_path = self._find_in_test_data_directories(filename)

But the function never returned None. It returned response.json() for 404
as well as 200, so a file the server cannot resolve came back as an error dict
where a path string was expected. _get_path_or_location() then handed that to
os.path.exists():

TypeError: stat: path should be string, bytes, os.PathLike or integer, not dict

which propagated out as a non-zero exit for the whole galaxy-tool-test
process, after results had already been written.

Note the old code was if response.status_code in [200, 404]: return result —
404 already returned rather than raised. This changes what comes back on
404, not whether 404 is fatal. A file that no strategy can resolve still fails
the test at
interactor.py#L600-L606
with Test input file (...) cannot be found. — which is the point, since that
message is actionable and the TypeError was not.

Also tidied

While _get_path_or_location() is being taught to handle None:

• it asked the server for the same path twice, and under the new contract
  the second lookup would 404 again and build a literal file://None
  location;
• the retained branch reads as a straight sequence now that the download
  fallback is shared, rather than nesting it inside an else.

Relationship to #23401

These are independent fixes to the same function, but they interact, and
this one is the safer of the two to merge first.

#23401 makes --force_path_paste actually take effect for non-composite
inputs. Today that branch never reaches
_get_path_or_location()'s if fname and force_path_paste: block, so it
never calls test_data_path() and is never exposed to the 404 above. Once
#23401 lands, it does — so any installation using --force_path_paste
against a server that cannot resolve a test file would go from quietly
downloading it to the TypeError, taking the process down.

Merging this first removes that exposure. Happy to combine them into one PR
instead if reviewers would rather see them together.

Tests

Seven tests in test/unit/tool_util/verify/test_interactor.py: 200 / 404 /
other-error on the function itself, plus _get_path_or_location() falling
back to a download rather than building file://None, asking the server
exactly once, and still path-pasting when the server does have the file.

Confirmed that three of them fail without the change.

[mvdbeek]

That doesn’t belong in the function docstring, that is just overly confusing details. At best an inline comment, however the assumption is also wrong. If the test data path doesn’t exist, that is an error, don’t pretend it should be None ?

You will surely want a non-zero exit code at the end ?

[afgane]

Docstring trimmed to the contract line in 8b52c3c, with the 404 reasoning moved to a short comment on the branch itself.

On the assumption though — I think None is already the contract here, it just wasn't honoured:

• test_data_download assigns this method's result to local_path and then checks it on the next line (interactor.py#L583-L585):

  local_path = self.test_data_path(tool_id, filename, tool_version=tool_version)
  
  if result is None and (local_path is None or not os.path.exists(local_path)):
      local_path = self._find_in_test_data_directories(filename)

  That caller was written against Optional[str].

• The pre-change code was if response.status_code in [200, 404]: return result, so 404 already returned rather than raised. What it returned was the error body, which callers then passed to os.path.exists() — hence TypeError: stat: path should be string, bytes, os.PathLike or integer, not dict. The change is to what comes back on 404, not to whether 404 is fatal.

Semantically I read test_data_path as "can the server read this from a path?", which is the first of three resolution strategies alongside the download endpoint and the local --test-data directories. A 404 means "not by this route" rather than "the file does not exist".

You will surely want a non-zero exit code at the end?

Yes, and that still happens. A file that no strategy can resolve reaches interactor.py#L600-L606:

if result is None:
    ...
    raise AssertionError(f"Test input file ({filename}) cannot be found.")

So a genuinely missing input still fails the test — with that message instead of the TypeError, which is the actual improvement I'm after.

That said, if your point is that 404 should raise and the existing in [200, 404] was itself the bug, that's a fair position — but it changes behaviour for anyone relying on the fallback, so I'd rather do it as its own PR than fold it in here. Happy to go that way if you prefer; just let me know which reading you meant.

[mvdbeek]

subclass this ?

[afgane]

Done in 8b52c3c — both helpers are now subclasses of GalaxyInteractorApi overriding _get / test_data_path / test_data_download, instead of building the instance with __new__ and assigning onto it.

That also removes all three # type: ignore[method-assign] comments, one of which was the lint failure on this branch earlier, so it takes a fragility out along with the ugliness.

[github-actions]

This PR was merged without a "kind/" label, please correct.