search

Roestlab / massdash

MassDash: A web-based dashboard for streamlined DIA-MS visualization, analysis, prototyping, and optimization
https://massdash.streamlit.app/
BSD 3-Clause "New" or "Revised" License
16 stars 3 forks source link

Test/add tests #114

Closed jcharkow closed 3 months ago

jcharkow commented 4 months ago

Description

Add/Update Tests

Fixes # (issue)

112

Type of change

Please delete options that are not relevant.

How Has This Been Tested?

Pytest has been run locally on ubuntu.

Checklist:

Other

Uncategorised!

jcharkow commented 4 months ago

@irahorecka or @singjc would either of you be able to help with the following?

TODO:

singjc commented 4 months ago

@irahorecka or @singjc would either of you be able to help with the following?

TODO:

* Needs testing on windows/mac

* Need to update github actions

* Need to add syrupy and pytest to testing installation

@irahorecka , would you be able to help out with this?

irahorecka commented 4 months ago

I could work on updating the GitHub Actions - is there a list of items that need to be updated?

jcharkow commented 4 months ago

I am not sure exactly how it is configured right now so I don't know exactly what needs to be updated.

My main concern is ensuring syrupy and pytest are downloaded before the tests begin. Not sure if we should create a requirements-test or something?

Also note I a massdash.testing that depends on syrupy. Not sure if this code should be placed in a different spot since syrupy is only needed for testing?

I could work on updating the GitHub Actions - is there a list of items that need to be updated?

jcharkow commented 4 months ago

@irahorecka any updates on this?

irahorecka commented 4 months ago

Hey sorry I’ve been really busy with other things. If there is a CI/CD portion I can update I’d be happy to do that. Haven’t looked into Syrupy yet. Do we need more tests?

irahorecka commented 4 months ago

Also, I notice we only test for Python 3.9, 3.10, and 3.11. We claim to cover 3.8 - 3.12. Should we narrow this range? I believe 3.8 isn't compatible with the Streamlit version we're using. I'm unsure about 3.12; I could give this a try right now.

[EDIT] Actually, the current PyOpenMS only supports 3.9, 3.10, and 3.11. I'll update the PyPI specs and README accordingly.

irahorecka commented 4 months ago

@jcharkow @singjc - all tests passed

jcharkow commented 4 months ago

@irahorecka thanks for fixing this. Are we using pytest/syrupy now? I could not tell. I'm not sure what command is currently being run by github action but I am not sure if the loader tests and peak picker tests are currently being discovered.

irahorecka commented 4 months ago

@jcharkow No problem; and no we are using Python's default unit test library:

python -m unittest discover -vv

I'm currently working on using pytest. But we have a testing issue: MS dataset paths, as defined in .ambr files, are hard coded to your computer's path. Can we safely delete this from the .ambr files or would you know if we could exclude adding this attribute altogether?

# serializer version: 1
# name: test_loadTransitionGroupFeature[INVALID-0]
  '''
  TransitionGroupFeatureCollection({
    '/home/joshua/Development/massdash_alt/massdash/test/test_data/xics/test_chrom_1.sqMass': list([
    ]),
    '/home/joshua/Development/massdash_alt/massdash/test/test_data/xics/test_chrom_2.sqMass': list([
    ]),
  })
  '''
# ---
jcharkow commented 4 months ago

good question, yes this is not ideal. maybe the current functionality does not make sense either and the dictionary key should only be the runname instead of the full file path? Alternatively we can ignore the keys and only serialize the values?

irahorecka commented 4 months ago

@jcharkow I raised an issue with Syrupy developers to see if it's possible to ignore the filename parameter when taking snapshots. I'm unsure when they'll reply, so if you'd like to merge this branch in its current state I'm okay with it.

jcharkow commented 4 months ago

Thanks for investing further. Thinking about it more I think that it might make sense to not use the whole path for the dictionary names to make testing easier. Then we can avoid this issue and also makes jupyter notebook interface easier.

irahorecka commented 4 months ago

No problem - I tried relative paths from the calling file to the data file but to no avail

irahorecka commented 4 months ago

I believe unit test is still useful for some tests?

jcharkow commented 4 months ago

I believe unit test is still useful for some tests?

I thought that pytest could also test for uniittests and it looks like it has prettier syntax?

irahorecka commented 4 months ago

Hmm - not sure!

irahorecka commented 3 months ago

Hey @jcharkow, here's the issue I raised with Syrupy developers: https://github.com/tophat/syrupy/issues/861

I will leave the merging of this branch up to you

jcharkow commented 3 months ago

Hey @jcharkow, here's the issue I raised with Syrupy developers: tophat/syrupy#861

I will leave the merging of this branch up to you

Thank you!

jcharkow commented 3 months ago

Note: there was some difficulty with tests on windows (test_MzMLDataAccess.py) because of the ionMobilityTest.mzML file not being windows compatible for OnDiscMSExperiment(). As a workaround, the file is opened as an MSExperiment() and resaved and the tests are done on the test file.

See the error ran into here: image

jcharkow commented 3 months ago

@singjc is this good to merge or would you like to review this before merging? @irahorecka reviewed this and said it was good to go.

jcharkow commented 3 months ago

Some Docs still have to be updated for this since syntax has changed however the GUI looks fine

singjc commented 3 months ago

@singjc is this good to merge or would you like to review this before merging? @irahorecka reviewed this and said it was good to go.

Yes, feel free to merge when ready.