Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐕 Batch: Unit Testing in Ersilia CLI #1319

Open
6 of 32 tasks
DhanshreeA opened this issue Oct 13, 2024 · 32 comments
Open
6 of 32 tasks

🐕 Batch: Unit Testing in Ersilia CLI #1319

DhanshreeA opened this issue Oct 13, 2024 · 32 comments
Labels
good first issue Good for newcomers

Comments

@DhanshreeA
Copy link
Member

DhanshreeA commented Oct 13, 2024

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:
  • is_input_header positive test @musasizivictoria
  • is_input_header negative test
  • is_key_header positive test @mercybassey
  • is_key_header negative test
  • _is_smiles positive test when Chem is None
  • _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)
  1. 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

@DhanshreeA DhanshreeA added the good first issue Good for newcomers label Oct 13, 2024
@DhanshreeA DhanshreeA moved this from On Hold to Queued in Ersilia Model Hub Oct 13, 2024
@musasizivictoria
Copy link
Contributor

musasizivictoria commented Oct 13, 2024

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

@DhanshreeA
Copy link
Member Author

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

@KimFarida
Copy link
Contributor

May I have the second one please ?

@musasizivictoria
Copy link
Contributor

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

@Ann-Clawson
Copy link

Add is_input_header negative test
#1321

@mercybassey
Copy link
Contributor

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

@DhanshreeA
Copy link
Member Author

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

@Ajoke23
Copy link
Contributor

Ajoke23 commented Oct 17, 2024

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

@aderemi1224
Copy link
Contributor

@

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

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

@DhanshreeA
Copy link
Member Author

@Ajoke23 go ahead! Please link your PR here.

@DhanshreeA
Copy link
Member Author

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

@DhanshreeA
Copy link
Member Author

@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
Copy link
Member Author

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

@DhanshreeA DhanshreeA moved this from Queued to In Progress in Ersilia Model Hub Oct 17, 2024
@daud99
Copy link
Contributor

daud99 commented Oct 17, 2024

@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
Copy link
Member Author

@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
Copy link
Contributor

daud99 commented Oct 17, 2024

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

daud99 pushed a commit to daud99/ersilia that referenced this issue Oct 17, 2024
* Implemented test case with empty and standard input.

* Updated README.md for input folder inside test.

* Changed the input file structure to organize test inputs better for different test cases.
@KimFarida
Copy link
Contributor

@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
Copy link
Contributor

daud99 commented Oct 17, 2024

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

@KimFarida
Copy link
Contributor

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

@KimFarida
Copy link
Contributor

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
Copy link
Contributor

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

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

@daud99
Copy link
Contributor

daud99 commented Oct 17, 2024

@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
Copy link
Contributor

@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
Copy link
Contributor

adebisi4145 commented Oct 18, 2024

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

@adebisi4145
Copy link
Contributor

adebisi4145 commented Oct 18, 2024

Add _is_inchikey negative test
PR: #1334

@Faith-Adegoke
Copy link
Contributor

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

@tongyu0924
Copy link
Contributor

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

@musasizivictoria
Copy link
Contributor

musasizivictoria commented Oct 21, 2024

@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
Copy link
Contributor

Ajoke23 commented Oct 21, 2024

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
Copy link
Contributor

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.

daud99 pushed a commit to daud99/ersilia that referenced this issue Oct 23, 2024
* Implemented test case with empty and standard input.

* Updated README.md for input folder inside test.

* Changed the input file structure to organize test inputs better for different test cases.
daud99 pushed a commit to daud99/ersilia that referenced this issue Oct 23, 2024
* Implemented test case with empty and standard input.
DhanshreeA added a commit that referenced this issue Nov 18, 2024
* Implemented test case with empty and standard input.

Co-authored-by: Daud Ahmed <[email protected]>
Co-authored-by: Dhanshree Arora <[email protected]>
@GemmaTuron
Copy link
Member

Hi @DhanshreeA

I see this as " In Progress" with no one assigned to it. I also marked it as priority 3. Can we better define this? If it is higher priority we should push for someone to tackle it, if it is low priority maybe put it on hold?

@DhanshreeA
Copy link
Member Author

We have made a lot of headway in implementing unit testing within Ersilia thanks to contributions from several people. I will make a small update here as to what is done and what is remaining, sometime this week. Until then, this is good to go back to being on hold.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
Status: On Hold
Development

No branches or pull requests