search

EnergySystemsModellingLab / MUSE_OS

Welcome to the MUSE-OS repository
https://muse-os.readthedocs.io/en/latest/
GNU General Public License v3.0
22 stars 8 forks source link

python -m pytest throws "10 skipped tests" #358

Open HarmonicReflux opened 1 week ago

HarmonicReflux commented 1 week ago

Should be tackle them individually, or, given they are skipped by default, get rid of them entirely?

alexdewar commented 1 week ago

Sorry I didn't see this sooner @HarmonicReflux. I've got notifications on for the repo, but seemingly I don't get notifications about new issues.

This issue could do with a bit more description of the problem. We don't always do this, but ideally we want to have some context provided so that we can have a discussion about if/when/how to tackle it.

Skipped tests aren't necessarily a problem per se -- the test suite was designed to skip certain tests if they can't be run for whatever reason. It's not the same as tests failing, which is an actual bug. That said, I think some of these skipped tests are candidates for removal and it's always good to clear away unused crud.

There seem to be three reasons why tests are skipped here:

  1. One of the tests needs another dependency installed and if you install it, the test isn't skipped anymore -- this one isn't a problem
  2. Some are skipped because they depend on a muse_legacy package -- not sure if/how we can acquire it or whether these tests are relevant anymore or even work
  3. Some are skipped because they won't work without a private dataset (called SGIModelData) -- again, they may no longer be relevant and may not work either

The last two sets aren't run by the CI system so we have no idea if they still work or not.

@ahawkes Do you have any thoughts about these last two points? If these tests are actually used by someone for something and are actually run sometimes, great, but otherwise we might as well just remove them.

ahawkes commented 1 week ago

Thanks - they could be used for the legacy version of MUSE - the original development which was very​ much a research effort where a bunch of things were tried. Ultimately a few bits were selected to be taken forward into the OS MUSE. But the thing is, the legacy version still exists, and a global model related to it is still being run by Sara Giarola in at least one live project (IAM COMPACT). So for now can we keep them but somehow flag that they are queued for removal after discussion with Sara.

From: Alex Dewar @.> Sent: 21 June 2024 10:57 To: EnergySystemsModellingLab/MUSE_OS @.> Cc: Hawkes, Adam D @.>; Mention @.> Subject: Re: [EnergySystemsModellingLab/MUSE_OS] python -m pytest throws "10 skipped tests" (Issue #358)

This email from @.*** originates from outside Imperial. Do not click on links and attachments unless you recognise the sender. If you trust the sender, add them to your safe senders listhttps://spam.ic.ac.uk/SpamConsole/Senders.aspx to disable email stamping for this address.

Sorry I didn't see this sooner @HarmonicRefluxhttps://github.com/HarmonicReflux. I've got notifications on for the repo, but seemingly I don't get notifications about new issues.

This issue could do with a bit more description of the problem. We don't always do this, but ideally we want to have some context provided so that we can have a discussion about if/when/how to tackle it.

Skipped tests aren't necessarily a problem per se -- the test suite was designed to skip certain tests if they can't be run for whatever reason. It's not the same as tests failing, which is an actual bug. That said, I think some of these skipped tests are candidates for removal and it's always good to clear away unused crud.

There seem to be three reasons why tests are skipped here:

  1. One of the tests needs another dependency installed and if you install it, the test isn't skipped anymore -- this one isn't a problem
  2. Some are skipped because they depend on a muse_legacy package -- not sure if/how we can acquire it or whether these tests are relevant anymore or even work
  3. Some are skipped because they won't work without a private dataset (called SGIModelData) -- again, they may no longer be relevant and may not work either

The last two sets aren't run by the CI system so we have no idea if they still work or not.

@ahawkeshttps://github.com/ahawkes Do you have any thoughts about these last two points? If these tests are actually used by someone for something and are actually run sometimes, great, but otherwise we might as well just remove them.

— Reply to this email directly, view it on GitHubhttps://github.com/EnergySystemsModellingLab/MUSE_OS/issues/358#issuecomment-2182425194, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AC37JLNCQTIN4RFO5TKDJITZIP2KJAVCNFSM6AAAAABJUAN2XGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBSGQZDKMJZGQ. You are receiving this because you were mentioned.Message ID: @.***>

alexdewar commented 1 week ago

Thanks for the response. There's no rush. And if someone actually is running these tests, then it totally makes sense to keep them.

We might as well ping Sara here. @sgiarols Do you have any thoughts?