ersilia-os / ersilia

The Ersilia Model Hub, a repository of AI/ML models for infectious and neglected disease research.
https://ersilia.io
GNU General Public License v3.0
225 stars 147 forks source link

šŸ• Batch: Unit Testing in Ersilia CLI #1319

Open DhanshreeA opened 1 month ago

DhanshreeA commented 1 month ago

Summary

We need to implement unit tests for the different components within Ersilia to make sure our pipelines are robust and we are able to push with confidence.

Due to the work of our fantastic contributors, we are expanding functionality within Ersilia that would definitely benefit from testing (PR linked as an example). This issue has a list of different components we could easily set up testing for. Please only pick one item, ie one checkbox` to work on and link your PR in this issue so we can track the ongoing work.

Objective(s)

  1. Tests for CompoundIdentifier class in ersilia/utils/identifiers/compound.py. Particularly, we need the following tests, and we will mock response from all the resolver URLs used in this class:

    • [x] is_input_header positive test @musasizivictoria
    • [x] is_input_header negative test
    • [x] is_key_header positive test @mercybassey
    • [x] is_key_header negative test
    • [x] _is_smiles positive test when Chem is None
    • [x] _is_smiles positive test when Chem is not None
    • [ ] _is_smiles negative test when Chem is None
    • [ ] _is_smiles negative test when Chem is not None
    • [ ] _is_inchikey positive test
    • [ ] _is_inchikey negative test
    • [ ] guess_type with inchikey
    • [ ] guess_type with smiles
    • [ ] guess_type with UNPROCESSABLE_INPUT
    • [ ] guess_type with whitespaces (\n, \t, etc) as input
    • [ ] guess_type with non character input (numbers, floats, random unicode characters)
    • [ ] _nci_smiles_to_inchikey positive test (we will mock this URL)
    • [ ] _nci_smiles_to_inchikey negative test (mock this URL again)
    • [ ] _pubchem_smiles_to_inchikey positive test (mock this URL)
    • [ ] _pubchem_smiles_to_inchikey negative test (mock this URL)
    • [ ] chemical_identifier_resolver with a correct input, eg: 'aspirin' which should return 'CC(=O)Oc1ccccc1C(O)=O'. (We will mock the URL with status code as 200 and response as given here)
    • [ ] chemical_identifier_resolver with incorrect input, eg: 'someincorrectinput' which should return a 404 response (This will be mocked)
    • [ ] chemical_identifier_resolver with incorrect inputs (\n, \t, NoneType, UNPROCESSABLE_INPUT)
    • [ ] encode with an invalid SMILES input when Chem is not None
    • [ ] encode with a valid SMILES input when Chem is not None
    • [ ] encode with incorrect inputs (\n, \t, NoneType, UNPROCESSABLE_INPUT)
    • [ ] encode with Chem being None with valid smiles and mocked response from self._pubchem_smiles_to_inchikey(smiles)
    • [ ] encode with Chem being None with valid smiles and mocked response from self._nci_smiles_to_inchikey(smiles)
  2. Tests for CatalogTable class in ersilia/hub/content/catalog.py file. In this we want to set up a global using a CatalogTable object catalog with some data and columns:

    • [ ] as_list_of_dicts
    • [ ] as_json
    • [ ] generate_separator_line
    • [ ] as_table
    • [ ] write

Documentation

No response

musasizivictoria commented 1 month ago

@DhanshreeA let me kindly give this(first checkbox) a shot as you reviewed well my work on the CompoundIdentifier class thanks

DhanshreeA commented 1 month ago

@musasizivictoria! Go for it! Please create a PR and link here.

KimFarida commented 1 month ago

May I have the second one please ?

musasizivictoria commented 1 month ago

@musasizivictoria! Go for it! Please create a PR and link here. PR here: https://github.com/ersilia-os/ersilia/pull/1320

Ann-Clawson commented 1 month ago

Add is_input_header negative test https://github.com/ersilia-os/ersilia/pull/1321

mercybassey commented 1 month ago

Hi @DhanshreeA Can I work on the third one is_key_header positive test?

DhanshreeA commented 1 month ago

@aderemi1224 would you like to tackle any of the tests here?

Ajoke23 commented 1 month ago

Hi @DhanshreeA can I pick is_key_header negative test to tackle?

aderemi1224 commented 1 month ago

@

@aderemi1224 would you like to tackle any of the tests here?

@DhanshreeA Yes I will, Please assign this to me. Thank you

DhanshreeA commented 1 month ago

@Ajoke23 go ahead! Please link your PR here.

DhanshreeA commented 1 month ago

@aderemi1224 you can work on any of the available tests! Thank you.

DhanshreeA commented 1 month ago

@KimFarida I had reacted to your comment to indicate yes you can work on any test you wish to take up. Please create a PR and link it here. Thank you!

DhanshreeA commented 1 month ago

@musasizivictoria feel free to pick up other tests if you're interested. :)

daud99 commented 1 month ago

@DhanshreeA I have already made completed my previous two tasks?

Can I have these tests assigned?

Tests for CatalogTable class in ersilia/hub/content/catalog.py file. In this we want to set up a global using a CatalogTable object catalog with some data and columns:

as_list_of_dicts as_json generate_separator_line as_table write

DhanshreeA commented 1 month ago

@daud99 welcome back! Indeed we want to create a CatalogTable object with some data of rows and columns. How about you start us off with creating a test file with this data fixture, and work on the first test (ie as_list_of_dicts) and we take it from there?

daud99 commented 1 month ago

@DhanshreeA Sounds great. Let's do it one by one. I'm on it.

KimFarida commented 1 month ago

@DhanshreeA I have already made completed my previous two tasks?

Can I have these tests assigned?

Tests for CatalogTable class in ersilia/hub/content/catalog.py file. In this we want to set up a global using a CatalogTable object catalog with some data and columns:

as_list_of_dicts as_json generate_separator_line as_table write

Haha i was working on those šŸ˜¬

daud99 commented 1 month ago

LOL @KimFarida I just made the pull request my bad.

KimFarida commented 1 month ago

@DhanshreeA i'd asked for the second one earlier but seeing as @daud99 has worked on them, on to the others not taken?šŸ˜¬

KimFarida commented 1 month ago

LOL @KimFarida I just made the pull request my bad.

All good šŸ˜Š. I guess i'll just wait on @DhanshreeA for my next course of action

KimFarida commented 1 month ago

LOL @KimFarida I just made the pull request my bad.

Quick one, did you write all 4 tests or just one?

daud99 commented 1 month ago

@KimFarida, I believe that instead of working on all four at once, it might be better to tackle them one by one. You could make a pull request for as_json, and for the one youā€™re working on, try to be explicit with the name instead of using pointers like "second" or "first." This way, it will be easier for others to understand what you're working on and reduce any confusion.

KimFarida commented 1 month ago

@KimFarida, I believe that instead of working on all four at once, it might be better to tackle them one by one. You could make a pull request for as_json, and for the one youā€™re working on, try to be explicit with the name instead of using pointers like "second" or "first." This way, it will be easier for others to understand what you're working on and reduce any confusion.

Yeah i figured that was my error, my bad. I did clarify on slack and she said to go aheadšŸ¤—. Thanks !

adebisi4145 commented 1 month ago

Hello @DhanshreeA , I picked _is_inchikey positive test PR: #1333

adebisi4145 commented 1 month ago

Add _is_inchikey negative test PR: #1334

Faith-Adegoke commented 1 month ago

Hi @DhanshreeA. I worked on the _is_smiles positive test when Chem is None. Pull Request is #1337.

tongyu0924 commented 1 month ago

Add test for nci_smiles_to_inchikey and positive inchikey. Pull Request is #1336 #1338

musasizivictoria commented 1 month ago

@DhanshreeA While looking at the _is_smiles tests(sppecifically negative test), I noticed that the method currently returns True for empty string inputs. Shouldnā€™t _is_smiles explicitly reject such cases as invalid; forexample

if not isinstance(text, str) or not text.strip(): return False

, similar to how non-string inputs are typically handled? Would adding a stricter input validationā€”like checking for empty or whitespace-only stringsā€”align better with the intended behavior of the method?

otherwise, currently, testing the method returns True for an empty string. sorry if this is already reported or addressed cc @Malikbadmus

Ajoke23 commented 1 month ago

Hi @DhanshreeA. I worked on the _is_smiles positive test when Chem is None. Pull Request is #1337.

Hi @Faith-Adegoke . I already worked on this previously #1332

Faith-Adegoke commented 1 month ago

Hi @DhanshreeA. I worked on the _is_smiles positive test when Chem is None. Pull Request is #1337.

Hi @Faith-Adegoke . I already worked on this previously #1332

Oh!. Sorry about that. I didn't see it.