Fix tests for baseUrl parameter in getRequestyModels #7336
Conversation
fixed URL construction for models and balance endpoints
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
dosubot
bot
added
size:L
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| const result = await getModels({ provider: "requesty", apiKey: DUMMY_REQUESTY_KEY }) | ||
|
|
||
| expect(mockGetRequestyModels).toHaveBeenCalledWith(DUMMY_REQUESTY_KEY) | ||
| expect(mockGetRequestyModels).toHaveBeenCalledWith(undefined, DUMMY_REQUESTY_KEY) |
There was a problem hiding this comment.
This test expects getRequestyModels to be called with (undefined, DUMMY_REQUESTY_KEY), but it doesn't actually test the baseUrl parameter properly. Could we add a test case that verifies baseUrl is passed correctly when it's provided in the options?
| expect(mockGetRequestyModels).toHaveBeenCalledWith(undefined, DUMMY_REQUESTY_KEY) | |
| it("calls getRequestyModels with baseUrl when provided", async () => { | |
| const mockModels = { /* ... */ } | |
| mockGetRequestyModels.mockResolvedValue(mockModels) | |
| const result = await getModels({ | |
| provider: "requesty", | |
| apiKey: DUMMY_REQUESTY_KEY, | |
| baseUrl: "https://custom.requesty.ai/v1" | |
| }) | |
| expect(mockGetRequestyModels).toHaveBeenCalledWith("https://custom.requesty.ai/v1", DUMMY_REQUESTY_KEY) | |
| expect(result).toEqual(mockModels) | |
| }) |
| return new URL(url).toString() | ||
| } catch (error) { | ||
| // If the provided baseUrl is invalid, fall back to the default | ||
| console.warn(`Invalid base URL "${baseUrl}", falling back to default`) |
There was a problem hiding this comment.
Is it intentional that we silently fall back to the default URL when an invalid baseUrl is provided? In development/test environments, throwing an error might help catch configuration issues early. What do you think about adding an environment check here?
| }) | ||
| }) | ||
|
|
||
| it("can use a base URL instead of the default", () => { |
There was a problem hiding this comment.
Could we make this test name more descriptive? Something like "initializes OpenAI client with custom base URL when provided" would be clearer about what's being tested.
| if (type === "router") { | ||
| return baseUrl | ||
| } else { | ||
| return baseUrl.replace("router", type).replace("v1", "") |
There was a problem hiding this comment.
The URL transformation logic here (replacing 'router' with service type and removing 'v1') seems fragile. What happens if the baseUrl doesn't contain 'router' or 'v1'? Should we add some validation or tests to ensure this works correctly for different URL formats?
hannesrudolph
added
the
Issue/PR - Triage
This PR fixes the failing tests that were broken after adding the baseUrl parameter to getRequestyModels function.
Changes
src/api/providers/__tests__/requesty.spec.tsto use a valid URL formatsrc/api/providers/fetchers/__tests__/modelCache.spec.tsto match the new parameter order (baseUrl, apiKey)Related to PR #7275
Important
Fix tests for
getRequestyModelsby updating URL handling and parameter order.requesty.spec.tsto use valid URL format forbaseUrl.modelCache.spec.tsto match new parameter order ingetRequestyModels().getRequestyModels()inrequesty.tsto acceptbaseUrland resolve it usingtoRequestyServiceUrl().getModels()inmodelCache.tsto passbaseUrltogetRequestyModels().toRequestyServiceUrl()inutils/requesty.tsto handle URL resolution forrequestyservices.This description was created by
for 64ac30d. You can customize this summary. It will automatically update as commits are pushed.