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

Fix maicos pipeline #102

Merged
merged 1 commit into from
Mar 19, 2024
Merged

Fix maicos pipeline #102

merged 1 commit into from
Mar 19, 2024

Conversation

hejamu
Copy link
Contributor

@hejamu hejamu commented Nov 26, 2023

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

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @hejamu !

@hejamu hejamu marked this pull request as draft November 26, 2023 17:56
@IAlibay
Copy link
Member

IAlibay commented Nov 26, 2023

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

hejamu commented Nov 26, 2023

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

IAlibay commented Nov 26, 2023

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

IAlibay commented Nov 26, 2023

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

hejamu commented Nov 26, 2023

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: tox-dev/tox#2858, and pinging @PicoCentauri as resident tox expert)

@PicoCentauri
Copy link

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

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

IAlibay commented Dec 24, 2023

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

IAlibay commented Mar 17, 2024

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

hejamu commented Mar 19, 2024

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

@IAlibay
Copy link
Member

IAlibay commented Mar 19, 2024

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

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

hejamu commented Mar 19, 2024

@IAlibay you are right, I was confused

@hejamu hejamu reopened this Mar 19, 2024
@hejamu hejamu marked this pull request as ready for review March 19, 2024 15:16
Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Thanks @hejamu!

@IAlibay IAlibay merged commit f78b804 into MDAnalysis:main Mar 19, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants