Fix: Avoid following PURL redirects in unit tests

[alexskr]

Updated PURL-related tests to assert only the HTTP 302 status and Location header, without following the redirect to the BioPortal UI. This avoids triggering Cloudflare bot protection during CI runs.

Resolves:

• Unit test fails in GitHub Actions due to Cloudflare bot protection — proposal to test only PURL redirect header #41

[jvendetti]

We do not want to test UI behavior here — we only verify that the
correct redirect is served by the PURL resolver. Testing how the UI handles or
rewrites URLs (e.g., with query parameters or concept ID fragments) is out of scope
for these tests.

I'm not sure I'm following this comment. These unit tests weren't added to test UI behavior. They were added because the purl method in this library was (in some cases) generating invalid PURLs that resulted in 404 errors. See this comment for more details. The fix had to be applied to the code in this library, hence the new unit tests.

[alexskr]

We do not want to test UI behavior here — we only verify that the
correct redirect is served by the PURL resolver. Testing how the UI handles or
rewrites URLs (e.g., with query parameters or concept ID fragments) is out of scope
for these tests.

I'm not sure I'm following this comment. These unit tests weren't added to test UI behavior. They were added because the purl method in this library was (in some cases) generating invalid PURLs that resulted in 404 errors. See this comment for more details. The fix had to be applied to the code in this library, hence the new unit tests.

You're right — the test likely wasn't originally intended to validate UI behavior. But in practice, it was doing exactly that.

Here’s what was happening:

The test called http://purl.bioontology.org/ontology/SNOMEDCT/64572001

The PURL server responded with a 302 redirect to
https://bioportal.bioontology.org/ontologies/SNOMEDCT/classes/64572001

The test followed that redirect to the UI, and the UI itself issued another redirect to
https://bioportal.bioontology.org/ontologies/SNOMEDCT?p=classes&conceptid=64572001

The test then asserted that the final URL matched that rewritten form — which is handled by the frontend.
So while the test may have been added to catch issues with purl generation, it ended up validating how the UI rewrites and serves the redirected URL, not just the correctness of the purl itself.

[mdorf]

The code changes themselves appear sound.

[jvendetti]

Yes -- I'm familiar with what's happening with the redirect chain, which is why I added the faraday-follow_redirects gem. Following the redirect chain allowed me to be certain that the system was functioning end-to-end and that the PURLs generated by this library didn't ultimately result in 404s in the Rails application.

[jvendetti]

Split PURL resolution tests into a new file test_class_purl.rb to separate concerns from core class model tests in test_class.rb

I disagree with moving these tests to a separate file. It's a best practice to have one test file per class/module. The tests are exercising the purl method, which is a member of Class. I don't see a reason to fragment the unit tests for Class, particularly when the file is less than 100 lines.

[alexskr]

Yes -- I'm familiar with what's happening with the redirect chain, which is why I added the faraday-follow_redirects gem. Following the redirect chain allowed me to be certain that the system was functioning end-to-end and that the PURLs generated by this library didn't ultimately result in 404s in the Rails application.

Validating the full redirect chain is sensible when the goal is to ensure that a PURL ultimately lands on a functioning UI page.

That said, I think this kind of end-to-end check might be better suited for an integration test on the UI side, rather than in the Ruby client’s unit test suite. My understanding is that from the perspective of this library, its responsibility is to retrieve a valid .purl from the API and ensure that it resolves; it is not responsible for testing how the UI handles or rewrites that redirect once it is received.

By limiting the test to just verifying the initial redirect from the PURL server (i.e., checking the Location header and status), we still catch broken or malformed PURLs without relying on frontend behavior or risking Cloudflare-related CI failures.

[alexskr]

Split PURL resolution tests into a new file test_class_purl.rb to separate concerns from core class model tests in test_class.rb

I disagree with moving these tests to a separate file. It's a best practice to have one test file per class/module. The tests are exercising the purl method, which is a member of Class. I don't see a reason to fragment the unit tests for Class, particularly when the file is less than 100 lines.

Totally fair — and I agree with the principle that test files generally map to class/module boundaries. Since purl is a method on Class, it's reasonable to keep tests in test_class.rb.

That said, my thinking here is based more on functional separation than method ownership. The purl tests — at least in their current form — are really validating the redirect behavior of the PURL service, not just how the client constructs .purl, it goes a step further by making a live request to the PURL and asserting that the redirect target is correct.

Given that, those tests felt conceptually separate from the core model tests, which focus on how the client loads and structures data returned from the API. Since this library’s primary role is to wrap API responses into objects for downstream use (e.g., in the UI), checking how an external redirect behaves feels like a different concern.

I think one way to do this more cleanly would be to move the simple .purl retrieval checks back into test_class.rb, since that’s part of what the class is directly responsible for. Then we could move the redirect resolution tests (i.e., the live HTTP calls and 302 checks) into a dedicated test file. That would keep the test suite aligned with both class boundaries and functional responsibilities, while also making it easier to skip or isolate external network tests for users who don’t rely on PURLs.

That’s the reasoning anyway; I’m not strongly opposed to keeping everything in one file if that’s the preferred approach. Just wanted to lay out the rationale in case it’s useful.

[alexskr]

@jvendetti, I reverted the unit test file split; it's easier that way.