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

Moved atlas_gen to separate package #29

Merged
merged 11 commits into from Jun 1, 2020
Merged

Moved atlas_gen to separate package #29

merged 11 commits into from Jun 1, 2020

Conversation

vigji
Copy link
Member

@vigji vigji commented May 31, 2020

In this PR:

  • I moved the atlas_gen module to be a separate package; this in principle should make sure there's no confusion on what should be imported in the main package that needs to be tested;
  • I removed 2 of the tests that could not be parallelelized;
  • I removed the installation of the dev tools for the tests.

@vigji vigji marked this pull request as draft May 31, 2020 10:55
@vigji
Copy link
Member Author

vigji commented May 31, 2020

Final test structure:

  • Linux (one version only): py3.6, 3.7, 3.8;
  • Windows: py3.7 only;
  • Separate test for linting, so black/flake8 errors are lumped together in a single place.

Tests are significantly faster now, although Windows still take some more time to setup. Maybe we could temporarily disable that together with macOS? I left all the code in the travis folder though, so put them back in the future will be straightforward.

I also added a separate requirements.txt file for easy deps parsing.

I think this should be mergeable now! @adamltyson ?

@vigji vigji requested a review from adamltyson May 31, 2020 12:21
@vigji vigji mentioned this pull request May 31, 2020
@vigji vigji marked this pull request as ready for review May 31, 2020 12:30
@adamltyson
Copy link
Member

I won't have time to review this properly until tomorrow, but it looks ok.

I'm keen to keep as much testing as we can though, because I've come across python version/OS interactions in the past, when wheels are only built for specific versions etc.

although Windows still take some more time to setup. Maybe we could temporarily disable that together with macOS?

I think we should absolutely keep the Windows test, and I would like to bring the macOS test back if we can.

Ideally we would also test every combination, but as this takes a long time, maybe we can keep it for tagged commits (i.e. to prevent deployment if there is an issue)?

Sorry to be a pain, but I think the tests taking a bit longer isn't much of an issue if we make sure everything passes locally (and maybe be selective with the commits we push?). I'm also wary that I will be maintaining quite a lot of software that will likely depend on this package, so I'm keen to test as much as is practical.

@vigji
Copy link
Member Author

vigji commented May 31, 2020

Ok, I added back the macOS test.

True that testing locally should take care of most problems. Still, maybe all the matrix is an overkill for all commits. If you know a way of trigger the job will all OSs and all versions only on marked commits that would be great!

@vigji
Copy link
Member Author

vigji commented Jun 1, 2020

I will marge for now as I don't see potential issues, but let me know if something's not ok. I am happy to reintroduce the grid testing if it can restricted to tags in the future!

@vigji vigji merged commit 5df3a13 into master Jun 1, 2020
@adamltyson
Copy link
Member

Cool, I've added this functionality in #30.

@vigji vigji deleted the fixing_tests branch August 21, 2020 10:58
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.

None yet

2 participants