search

pepkit / peppy

Project metadata manager for PEPs in Python
https://pep.databio.org/peppy
BSD 2-Clause "Simplified" License
37 stars 13 forks source link

Apply bugfix #409

Closed rafalstepien closed 2 years ago

rafalstepien commented 2 years ago

All the reasons described in #408

closes #408

nleroy917 commented 2 years ago

@rafalstepien this looks good to me. Wondering if we can bring in #410 with this? Should be only a line change. This is useful to me in pepingester so I can set a default PEP version if one doesn't exist.

This actually will be helpful with pephub as well.

rafalstepien commented 2 years ago

Sure, if there is any better place where you store unittests then let me know!

rafalstepien commented 2 years ago

@rafalstepien this looks good to me. Wondering if we can bring in #410 with this? Should be only a line change. This is useful to me in pepingester so I can set a default PEP version if one doesn't exist.

This actually will be helpful with pephub as well.

Sure, we can incorporate that.

nleroy917 commented 2 years ago

I was doing something wrong... nvm 🤦🏻‍♂️

Needs to be from peppy.const import PEP_LATEST_VERSION

rafalstepien commented 2 years ago

@nsheff I would appreciate the review and decision what to do about test file (mentioned by @vreuter )

nsheff commented 2 years ago

@rafalstepien I think this looks like some legacy organization, but it doesn't really make sense -- please just move any tests that you classify as "unit tests" outside of the "smoketests" folder? If there are tests in there that qualify as smoketests, then you can leave them there.

In the meantime, @stolarczyk can you clarify the distinction you were going for with putting these tests under "smoketests"? At first glance it looked like most of these are just unit tests.

nsheff commented 2 years ago

@rafalstepien some updates for your example:

  1. Examples should not be added directly to this repository, which is a read-only mirror of example_peps. Please see: https://github.com/pepkit/peppy/blob/master/update_test_data.sh
  2. If the above was not clear to you, can you please write instructions to this effect somewhere where you would have found them, for the next developer?
  3. So you'll be adding a new example to that repo. Don't name that example "nextflow error" -- it isn't going to be an error in perpetuity. Come up with a name that makes sense for the example.
stolarczyk commented 2 years ago

I decided to smoke test peppy to save time as we were rushing for a release at some point.

To me, smoke testing means testing the entire peppy.Project initialization process and inspecting the result, which includes config parsing, preprocessing, applying the attribute modifiers etc. as opposed to testing individual functions that perform these actions. Maybe that's an incorrect classification.

nsheff commented 2 years ago

Thanks for the clarification @stolarczyk . I think that's fine.

So @rafalstepien -- please just classify your new tests as either smoketests (following michal's definition), in which case put them under smoketests/, or as unit tests, in which case you should remove them and put them in the test or under some subfolder other than smoketests.

rafalstepien commented 2 years ago

So I moved most of the tests that in my opinion are unittests to apropriate file, removed redundancy from fixtures and renamed some example files. I will merge this after familiarizing with how to correctly add examples using the script you mentioned. Thank you all for help!