Thanks for the feedback! Those options for splitting seem reasonable. I'll work on setting everything up and issuing the offshoot PRs.
-Jared ________________________________ From: Madicken Munk email@example.com Sent: Friday, December 6, 2019 9:25 AM To: firstname.lastname@example.org email@example.com Subject: [yt-dev] Re: Switching from Nose to Pytest
I would like to add my support for splitting this up into multiple PRs for review. I personally don't feel I would give an adequate review of it as a single PR, and this is. I think the options that Nathan has proposed for splitting seem like a reasonable approach to splitting.
Waiting until things are in a relatively stable state to split also makes sense.
I'm looking forward to helping review this work.
On Fri, Dec 6, 2019 at 7:21 AM Matthew Turk <firstname.lastname@example.org:email@example.com> wrote: Hey Nathan,
On Thu, Dec 5, 2019 at 7:04 PM Nathan Goldbaum <firstname.lastname@example.org:email@example.com> wrote:
I just happened to look at the PR today and noticed it was really big.
Is the plan still to re-issue it in chunks?
I thought so, yes. I think the reason it hasn't been is that it's still in progress and being developed, so the cut-off for the different PRs would, at this point, be pretty unclear and require a ton of overhead of merging and cherry-picking between branches.
Right now I think this PR is too big to review without glossing over things by any realistic reviewer.
I still think the most straightforward way to review this is in stages:
- PR adds configuration for testing with pytest and sets up CI to use both pytest and nose, at first pytest doesn’t run any tests.
- Convert unit tests to pytest. If this ends up being huge as well then the conversion happens module by module to keep things reviewable.
- Convert answer tests to pytest. Again if this is huge it should happen module by module.
- Remove nose from CI runners.
The tests are a critical piece of infrastructure and I think it would be a bad idea to do such a big transition without careful review by people who haven’t been involved with the code development for this.
Also btw what’s the status of the YTEP? Should I give it another review pass? The YTEP should probably be merged before we merge the implementation so we get a sign-off that the design is OK.
On Tue, Oct 1, 2019 at 1:51 PM Nathan <firstname.lastname@example.org:email@example.com> wrote:
Yeah, making the unit tests run under pytest seems like a pretty easy win!
On Tue, Oct 1, 2019 at 1:44 PM Matthew Turk <firstname.lastname@example.org:email@example.com> wrote:
I think that the three stages of PR might not be useful, but I do agree that a YTEP and having at least two stages of PR would be good. Especially since the process of converting *answer* tests, which I *think* is where most of the changes are, will require actually converting the files from one to the other. I believe that as-is, the *unit* tests work in both (except where they still yield). So I'd suggest two PRs -- one that sets up the pytest infrastructure, and one that converts answer tests to it.
On Mon, Sep 30, 2019 at 12:14 PM Coughlin, Jared W <firstname.lastname@example.org:email@example.com> wrote:
Thank you for the suggestions! I agree with all of them. I didn't realize the PR would get so large when I first started it, so I agree that breaking it up will make the integration process much smoother overall. I'll get started on a YTEP today and let you know if I have any questions. Thanks!
-Jared ________________________________ From: Nathan <firstname.lastname@example.org:email@example.com> Sent: Monday, September 30, 2019 11:07 AM To: firstname.lastname@example.org:email@example.com <firstname.lastname@example.org:email@example.com> Subject: [yt-dev] Re: Switching from Nose to Pytest
I think one thing that might help guide this discussion and make reviewing your pull request easier is for you to write up a YTEP document that goes a little bit into the technical and social details behind this change. Things that would be good to hear about:
- What do future contributors need to do to add answer tests in the new framework?
- What are differences between nose and pytest and developers need to be aware of?
- Are there any downsides to making this change?
- Any technical details that might be helpful for reviewers of your pull request to read in a more narrative fashion than is possible in a pull request.
Writing up a YTEP is a great way to both document the change in a format that will be preserved long-term and a way for you to explain what's happening without others needing to read over all the code changes. It's also a good thing for your personal resume to be able to point to a design document for a major development push along with the code and pull request(s).
Separately from the YTEP, I'd like to encourage you to think about breaking up your pull request into multiple chunks that can be upstreamed one at a time. This might mean running both pytest and nose for a while as things get converted. I could imagine this going like, first pull request sets up the pytest infrastructure and gets pytest running but with no actual tests running under pytest. Second pull request converts some subsection of the code to run under pytest. Third pull request does another subsection and so on until the entire codebase is converted. That will probably make it much easier for reviewers to understand your changes and make it easier for reviewers to spot bugs and possible issues.
I understand the desire to upstream something like this all at once but I think there's a big concern about making big changes to the codebase that are difficult to review all at once.
Hope that's helpful, please feel free to ping me on github if you want my help with anything.
On Mon, Sep 30, 2019 at 9:29 AM Matthew Turk <firstname.lastname@example.org:email@example.com> wrote:
Thanks for doing all of this.
One of the big things in this, that I think everybody should know about, is that it also changes our answer testing to use flat hashes of array values, instead of *full* array values that are stored in some remote location. This means that the results of the answer tests are co-located with the code, and much easier for folks to set up and use.
On Sat, Sep 28, 2019 at 10:31 AM Coughlin, Jared W <firstname.lastname@example.org:email@example.com> wrote:
I've been working on a PR (https://github.com/yt-project/yt/pull/2286) that converts yt's testing framework from nose to pytest. I believe that it's mostly ready to go, so I was writing to see what everyone's thoughts on this change are, if there is any feedback on my implementation, and if anyone would be willing to help review it, since there are quite a few changes. Thanks, and have a good weekend!
-Jared _______________________________________________ yt-dev mailing list -- firstname.lastname@example.org:email@example.com To unsubscribe send an email to firstname.lastname@example.org:email@example.com