Skip to content

Fix jobs methods to use function filter correctly#76

Merged
ElePT merged 3 commits intomainfrom
add-function-filter
Feb 6, 2026
Merged

Fix jobs methods to use function filter correctly#76
ElePT merged 3 commits intomainfrom
add-function-filter

Conversation

@ElePT
Copy link
Collaborator

@ElePT ElePT commented Jan 26, 2026

Summary

This PR addresses a discrepancy in the arguments of the jobs method compared to IBMServerlessClient. In order to be able to test the fix, this PR also includes a quick refactoring of the unit tests, including:

  • renaming folder to test to follow qiskit standards
  • separating tests into 2 files (one for catalog, one for serverless)
  • separating tests into different methods

Details and comments

@ElePT ElePT force-pushed the add-function-filter branch from 148c647 to 8611a03 Compare February 6, 2026 10:16
ElePT added 2 commits February 6, 2026 11:17
… into distict files to reflect file structure in the catalog. Split tests by tested functionality.
@ElePT ElePT force-pushed the add-function-filter branch from 8611a03 to ca66bd1 Compare February 6, 2026 10:17
@ElePT ElePT marked this pull request as ready for review February 6, 2026 10:29
@ElePT ElePT requested a review from a team as a code owner February 6, 2026 10:29
@ElePT ElePT changed the title Update jobs methods to use new filters Fix jobs methods to use function filter correctly Feb 6, 2026
@ElePT ElePT added the bug Something isn't working label Feb 6, 2026
@HuangJunye HuangJunye self-assigned this Feb 6, 2026
Copy link
Contributor

@HuangJunye HuangJunye left a comment

Choose a reason for hiding this comment

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

Everything looks very clear and correct to me. However, I am not very familiar with mocking test so I can't be certain that everything is ok so I am leaving only a comment not an approval.

I was a bit confused at first why test_catalog.py and test_serverless.py are so similar, but it just reflects again that there are two many similar classes and we need to refactor to simplify things.

@ElePT
Copy link
Collaborator Author

ElePT commented Feb 6, 2026

I was a bit confused at first why test_catalog.py and test_serverless.py are so similar, but it just reflects again that there are two many similar classes and we need to refactor to simplify things.

We could definitely work on a testing mixer or something of the sorts to avoid code repetition, but to be honest, with 2 tests I didn't want to over-engineer it. In the end, as you said, this reflects the fact that we are not taking advantage of inheritance to speed up development or testing, and we need to keep testing every component independently.

@avilches
Copy link
Contributor

avilches commented Feb 6, 2026

Everything looks very clear and correct to me. However, I am not very familiar with mocking test so I can't be certain that everything is ok so I am leaving only a comment not an approval.

I was a bit confused at first why test_catalog.py and test_serverless.py are so similar, but it just reflects again that there are two many similar classes and we need to refactor to simplify things.

Hi. The test_serverless.py module shouldn't exist in this project, right?. I mean, in this project, the ServerlessClient is just a third party dependency that will likely disappear soon. The QiskitFunctionsCatalog is the only class we should test.

EDIT: Nevermind. I thought the ServerlessClient was the IBMServerlessClient.

@ElePT ElePT added this pull request to the merge queue Feb 6, 2026
Merged via the queue into main with commit 5eb138f Feb 6, 2026
11 checks passed
@HuangJunye HuangJunye deleted the add-function-filter branch February 10, 2026 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants