search

MDAnalysis / MDAKits

The MDAnalysis Toolkits Registry
https://mdakits.mdanalysis.org
Other
15 stars 24 forks source link

Fix maicos pipeline #102

Closed hejamu closed 6 months ago

hejamu commented 9 months ago

The way we call our tests is now slightly different. This PR reflects this change.

IAlibay commented 9 months ago

@hejamu I see you've put this as draft, please feel free to ping me for re-review once you've finished any further changes.

hejamu commented 9 months ago

@IAlibay I forgot something important. We would now need two different commands for latest and develop, at least until a new tag is created... Is there a neat way to do that?

IAlibay commented 9 months ago

@IAlibay I forgot something important. We would now need two different commands for latest and develop, at least until a new tag is created... Is there a neat way to do that?

Ahh I see what you mean here. I didn't really anticipate this case (I somewhat naively assumed kits would have rapid release cycles).

I'll have to do some digging a bit later to see what's possible, it might have to be something I include as part of the upcoming registry refactor.

IAlibay commented 9 months ago

I am also a bit concerned that the latest run didn't actually throw an error even through the tests don't appear to have run. That's something to investigate separately.

hejamu commented 9 months ago

I am also a bit concerned that the latest run didn't actually throw an error even through the tests don't appear to have run. That's something to investigate separately.

That it a weird tox thing, since the environment tests does not exist it just exits with exit code 0... (some context: https://github.com/tox-dev/tox/issues/2858, and pinging @PicoCentauri as resident tox expert)

PicoCentauri commented 9 months ago

That is indeed weird. Also we have an additional check that should fail if the environment does not exist.

https://gitlab.com/maicos-devel/maicos/-/blob/main/tox.ini?ref_type=heads#L26-27

PicoCentauri commented 9 months ago

@IAlibay I forgot something important. We would now need two different commands for latest and develop, at least until a new tag is created... Is there a neat way to do that?

We can also release a new version soonish. Maybe this is easier and we want to this in any case, right @hejamu 😉?

I am also a bit concerned that the latest run didn't actually throw an error even through the tests don't appear to have run. That's something to investigate separately.

Seems like you can run tox -e py39-tests or tox -e tests successfully even though it is not specified explicitly in the config file. The CI did this and I was also able to run the tests successful with the current main branch.

IAlibay commented 8 months ago

@hejamu - it looks like things fixed themselves? (or at least CI started passing again). Is the change here still needed?

On my end I've not had the time to refactor the testing pipelines yet, I'll hopefully do so soon.

IAlibay commented 6 months ago

@hejamu since maicos had a release recently, this should work?

I'll open a separate issue about allowing for separate develop/latest test requirements.

hejamu commented 6 months ago

Yes, this should not be a problem right now. We can close this PR.

IAlibay commented 6 months ago

@hejamu I think we have to merge this to fix the problem? (i.e. we shouldn't be calling py39-tests anymore it seems?). If you can update this branch I'm happy to merge this in.

pep8speaks commented 6 months ago

Hello @hejamu! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 340:13: E129 visually indented line with same indent as next logical line

hejamu commented 6 months ago

@IAlibay you are right, I was confused