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

Move a lot of small build jobs into tests #1276

Merged
merged 1 commit into from
May 15, 2018
Merged

Conversation

DRMacIver
Copy link
Member

(Built on top of #1274 so that the build actually works. History will be much cleaner once that's merged and this is rebased, so you may want to hold off on reviewing until then).

One of @alexwlchan's suggestions for build time improvements in #1224 was that we should have fewer small build jobs. This PR adopts the principle that most of these little build jobs are basically tests, and we have this perfectly serviceable test runner right here...

We will go full galaxy brain when we start decorating some of these tests with @given, but thankfully we're not there yet.

@Zac-HD
Copy link
Member

Zac-HD commented May 11, 2018

This PR adopts the principle that most of these little build jobs are basically tests, and we have this perfectly serviceable test runner right here... We will go full galaxy brain when we start decorating some of these tests with @given

Just had trouble explaining to a friend why I was laughing at my laptop... More seriously, I actually think this is a decent way to approach the problem - we do indeed have a test runner, and might as well use it!

However: I would move as much as possible out of custom tooling, by leaning on plugins such as pytest-flake8 and pytest-mypy, with the goal of making it trivial to run these checks locally without the build stack (e.g. on Windows). Having a one-line pytest command that does a reasonable set of checks would make me a lot faster than waiting on CI all the time 😄

Moving the docs, linting, and formatting jobs in would be similarly nice and would be mostly covered by that suggestion.

@DRMacIver
Copy link
Member Author

However: I would move as much as possible out of custom tooling

You're more than welcome to. 😉

Joking aside, I'm more than happy for us to do that, but the task of actually trying to make the build sensible and cross-platform is a significant work in progress and every time I try to bite of a large chunk it spirals out of my control, so I'm trying to push back really hard on anything that's not very incremental. I'm more or less trying to make a beeline for getting hypothesis-ruby merged into the main repo as soon as possible and if I stop and do all of the big refactoring jobs that present themselves along the way it'll be some time in 2019.

@Zac-HD
Copy link
Member

Zac-HD commented May 11, 2018

Right, whoops, we do have that formal guideline against asking for expanded scope of PRs. Leave it out of this one and I'll take a go later, and probably rebase #1270 on top of that! (I have to sort out a testing + mypy environment for it anyway, so...)

@DRMacIver
Copy link
Member Author

One thing to note is that we actually shouldn't need to move it out of the custom tooling to get windows support. One of the nice thing about the tool refactor is that there's very little that is actually unsupportable on Windows right now. It's really just the installation code that is unixy right now, and if we can assume that Windows users have anaconda installed (which I think is fine to do) then it shouldn't be too hard to make that work on Windows too and provide a build.ps1 script or whatever then the build environment should work fine there too.

Not that I have a problem with relying on some more standard plugins to do the work for us if that's what you prefer, but given that we'd like to get the build environment working more broadly anyway...

@Zac-HD Zac-HD force-pushed the DRMacIver/tooling-tests branch 2 times, most recently from 3017aec to be187cf Compare May 15, 2018 03:15
Most of these little build jobs are basically tests, and we have this perfectly serviceable test runner right here...
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

I've rebased this on master to include the changes from #1274, #1275, #1278, and #1279 - it's been a big week for our build processes!

The only other change is to remove the check-rst job from .travis.yml, since it's implemented as part of the check-whole-repo-tests job.

@Zac-HD Zac-HD merged commit 7c53b8d into master May 15, 2018
@Zac-HD Zac-HD deleted the DRMacIver/tooling-tests branch May 15, 2018 05:01
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