Switching from Nose to Pytest
Hello!
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
Hi Jared,
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.
-Matt
On Sat, Sep 28, 2019 at 10:31 AM Coughlin, Jared W jcoughl3@illinois.edu wrote:
Hello!
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 -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
Hi Jared,
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.
-Nathan
On Mon, Sep 30, 2019 at 9:29 AM Matthew Turk matthewturk@gmail.com wrote:
Hi Jared,
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.
-Matt
On Sat, Sep 28, 2019 at 10:31 AM Coughlin, Jared W jcoughl3@illinois.edu wrote:
Hello!
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 -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
Hi Nathan,
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 nathan.goldbaum@gmail.com Sent: Monday, September 30, 2019 11:07 AM To: yt-dev@python.org yt-dev@python.org Subject: [yt-dev] Re: Switching from Nose to Pytest
Hi Jared,
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.
-Nathan
On Mon, Sep 30, 2019 at 9:29 AM Matthew Turk <matthewturk@gmail.commailto:matthewturk@gmail.com> wrote: Hi Jared,
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.
-Matt
On Sat, Sep 28, 2019 at 10:31 AM Coughlin, Jared W <jcoughl3@illinois.edumailto:jcoughl3@illinois.edu> wrote:
Hello!
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 -- yt-dev@python.orgmailto:yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.orgmailto:yt-dev-leave@python.org
_______________________________________________ yt-dev mailing list -- yt-dev@python.orgmailto:yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.orgmailto:yt-dev-leave@python.org
Hi all,
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 jcoughl3@illinois.edu wrote:
Hi Nathan,
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 nathan.goldbaum@gmail.com Sent: Monday, September 30, 2019 11:07 AM To: yt-dev@python.org yt-dev@python.org Subject: [yt-dev] Re: Switching from Nose to Pytest
Hi Jared,
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.
-Nathan
On Mon, Sep 30, 2019 at 9:29 AM Matthew Turk matthewturk@gmail.com wrote:
Hi Jared,
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.
-Matt
On Sat, Sep 28, 2019 at 10:31 AM Coughlin, Jared W jcoughl3@illinois.edu wrote:
Hello!
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 -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
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 matthewturk@gmail.com wrote:
Hi all,
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 jcoughl3@illinois.edu wrote:
Hi Nathan,
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 nathan.goldbaum@gmail.com Sent: Monday, September 30, 2019 11:07 AM To: yt-dev@python.org yt-dev@python.org Subject: [yt-dev] Re: Switching from Nose to Pytest
Hi Jared,
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.
-Nathan
On Mon, Sep 30, 2019 at 9:29 AM Matthew Turk matthewturk@gmail.com
wrote:
Hi Jared,
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.
-Matt
On Sat, Sep 28, 2019 at 10:31 AM Coughlin, Jared W jcoughl3@illinois.edu wrote:
Hello!
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 -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
Hi all,
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?
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:
1) 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. 2) Convert unit tests to pytest. If this ends up being huge as well then the conversion happens module by module to keep things reviewable. 3) Convert answer tests to pytest. Again if this is huge it should happen module by module. 4) 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.
Nathan
On Tue, Oct 1, 2019 at 1:51 PM Nathan nathan.goldbaum@gmail.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 matthewturk@gmail.com wrote:
Hi all,
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 jcoughl3@illinois.edu wrote:
Hi Nathan,
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 nathan.goldbaum@gmail.com Sent: Monday, September 30, 2019 11:07 AM To: yt-dev@python.org yt-dev@python.org Subject: [yt-dev] Re: Switching from Nose to Pytest
Hi Jared,
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.
-Nathan
On Mon, Sep 30, 2019 at 9:29 AM Matthew Turk matthewturk@gmail.com
wrote:
Hi Jared,
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.
-Matt
On Sat, Sep 28, 2019 at 10:31 AM Coughlin, Jared W jcoughl3@illinois.edu wrote:
Hello!
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 -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
Hey Nathan,
On Thu, Dec 5, 2019 at 7:04 PM Nathan Goldbaum nathan.goldbaum@gmail.com wrote:
Hi all,
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.
Nathan
On Tue, Oct 1, 2019 at 1:51 PM Nathan nathan.goldbaum@gmail.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 matthewturk@gmail.com wrote:
Hi all,
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 jcoughl3@illinois.edu wrote:
Hi Nathan,
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 nathan.goldbaum@gmail.com Sent: Monday, September 30, 2019 11:07 AM To: yt-dev@python.org yt-dev@python.org Subject: [yt-dev] Re: Switching from Nose to Pytest
Hi Jared,
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.
-Nathan
On Mon, Sep 30, 2019 at 9:29 AM Matthew Turk matthewturk@gmail.com wrote:
Hi Jared,
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.
-Matt
On Sat, Sep 28, 2019 at 10:31 AM Coughlin, Jared W jcoughl3@illinois.edu wrote:
Hello!
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 -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
Hi all,
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.
-Madicken
On Fri, Dec 6, 2019 at 7:21 AM Matthew Turk matthewturk@gmail.com wrote:
Hey Nathan,
On Thu, Dec 5, 2019 at 7:04 PM Nathan Goldbaum nathan.goldbaum@gmail.com wrote:
Hi all,
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.
Nathan
On Tue, Oct 1, 2019 at 1:51 PM Nathan nathan.goldbaum@gmail.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 matthewturk@gmail.com
wrote:
Hi all,
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 jcoughl3@illinois.edu wrote:
Hi Nathan,
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 nathan.goldbaum@gmail.com Sent: Monday, September 30, 2019 11:07 AM To: yt-dev@python.org yt-dev@python.org Subject: [yt-dev] Re: Switching from Nose to Pytest
Hi Jared,
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.
-Nathan
On Mon, Sep 30, 2019 at 9:29 AM Matthew Turk matthewturk@gmail.com
wrote:
Hi Jared,
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.
-Matt
On Sat, Sep 28, 2019 at 10:31 AM Coughlin, Jared W jcoughl3@illinois.edu wrote:
Hello!
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 -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org
Hi everyone,
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 madicken.munk@gmail.com Sent: Friday, December 6, 2019 9:25 AM To: yt-dev@python.org yt-dev@python.org Subject: [yt-dev] Re: Switching from Nose to Pytest
Hi all,
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.
-Madicken
On Fri, Dec 6, 2019 at 7:21 AM Matthew Turk <matthewturk@gmail.commailto:matthewturk@gmail.com> wrote: Hey Nathan,
On Thu, Dec 5, 2019 at 7:04 PM Nathan Goldbaum <nathan.goldbaum@gmail.commailto:nathan.goldbaum@gmail.com> wrote:
Hi all,
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.
Nathan
On Tue, Oct 1, 2019 at 1:51 PM Nathan <nathan.goldbaum@gmail.commailto:nathan.goldbaum@gmail.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 <matthewturk@gmail.commailto:matthewturk@gmail.com> wrote:
Hi all,
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 <jcoughl3@illinois.edumailto:jcoughl3@illinois.edu> wrote:
Hi Nathan,
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 <nathan.goldbaum@gmail.commailto:nathan.goldbaum@gmail.com> Sent: Monday, September 30, 2019 11:07 AM To: yt-dev@python.orgmailto:yt-dev@python.org <yt-dev@python.orgmailto:yt-dev@python.org> Subject: [yt-dev] Re: Switching from Nose to Pytest
Hi Jared,
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.
-Nathan
On Mon, Sep 30, 2019 at 9:29 AM Matthew Turk <matthewturk@gmail.commailto:matthewturk@gmail.com> wrote:
Hi Jared,
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.
-Matt
On Sat, Sep 28, 2019 at 10:31 AM Coughlin, Jared W <jcoughl3@illinois.edumailto:jcoughl3@illinois.edu> wrote:
Hello!
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 -- yt-dev@python.orgmailto:yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.orgmailto:yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.orgmailto:yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.orgmailto:yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.orgmailto:yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.orgmailto:yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.orgmailto:yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.orgmailto:yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.orgmailto:yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.orgmailto:yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.orgmailto:yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.orgmailto:yt-dev-leave@python.org
_______________________________________________ yt-dev mailing list -- yt-dev@python.orgmailto:yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.orgmailto:yt-dev-leave@python.org
Hi everyone,
Thank you all for the feedback, I really appreciate it! I'm writing to provide an update on the current thoughts about how best to proceed with reviewing and merging this large PR (https://github.com/yt-project/yt/pull/2286), and to open a discussion about it to see what everyone thinks. [https://avatars2.githubusercontent.com/u/20566579?s=400&v=4]https://github.com/yt-project/yt/pull/2286 [WIP] Pytest by jcoughlin11 · Pull Request #2286 · yt-project/ythttps://github.com/yt-project/yt/pull/2286 PR Summary This Work in Progress PR is meant to address answer testing in yt. Currently, yt uses nose for its testing. Additionally, the answer tests are quite time and memory intensive, at the mo... github.com
The idea right now is to issue several smaller PRs that are easier to review. Once each one is reviewed and accepted, it will be merged into master. This will result, for a time, in both nose and pytest coexisting in the code base and the number of tests being run by nose slowly reduced as additional PRs are merged. This also serves to keep everything more easily in sync with master.
The first of these smaller PRs, pytest infrastructure, can be found here: https://github.com/yt-project/yt/pull/2401 and it ports over three key changes that lay the groundwork for using pytest and the new hashing scheme. Thanks, and happy holidays!
Sincerely, -Jared ________________________________ From: Coughlin, Jared W jcoughl3@illinois.edu Sent: Friday, December 6, 2019 4:21 PM To: Madicken Munk madicken.munk@gmail.com; yt-dev@python.org yt-dev@python.org Subject: Re: [yt-dev] Re: Switching from Nose to Pytest
Hi everyone,
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 madicken.munk@gmail.com Sent: Friday, December 6, 2019 9:25 AM To: yt-dev@python.org yt-dev@python.org Subject: [yt-dev] Re: Switching from Nose to Pytest
Hi all,
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.
-Madicken
On Fri, Dec 6, 2019 at 7:21 AM Matthew Turk <matthewturk@gmail.commailto:matthewturk@gmail.com> wrote: Hey Nathan,
On Thu, Dec 5, 2019 at 7:04 PM Nathan Goldbaum <nathan.goldbaum@gmail.commailto:nathan.goldbaum@gmail.com> wrote:
Hi all,
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.
Nathan
On Tue, Oct 1, 2019 at 1:51 PM Nathan <nathan.goldbaum@gmail.commailto:nathan.goldbaum@gmail.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 <matthewturk@gmail.commailto:matthewturk@gmail.com> wrote:
Hi all,
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 <jcoughl3@illinois.edumailto:jcoughl3@illinois.edu> wrote:
Hi Nathan,
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 <nathan.goldbaum@gmail.commailto:nathan.goldbaum@gmail.com> Sent: Monday, September 30, 2019 11:07 AM To: yt-dev@python.orgmailto:yt-dev@python.org <yt-dev@python.orgmailto:yt-dev@python.org> Subject: [yt-dev] Re: Switching from Nose to Pytest
Hi Jared,
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.
-Nathan
On Mon, Sep 30, 2019 at 9:29 AM Matthew Turk <matthewturk@gmail.commailto:matthewturk@gmail.com> wrote:
Hi Jared,
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.
-Matt
On Sat, Sep 28, 2019 at 10:31 AM Coughlin, Jared W <jcoughl3@illinois.edumailto:jcoughl3@illinois.edu> wrote:
Hello!
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 -- yt-dev@python.orgmailto:yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.orgmailto:yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.orgmailto:yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.orgmailto:yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.orgmailto:yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.orgmailto:yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.orgmailto:yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.orgmailto:yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.orgmailto:yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.orgmailto:yt-dev-leave@python.org
yt-dev mailing list -- yt-dev@python.orgmailto:yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.orgmailto:yt-dev-leave@python.org
_______________________________________________ yt-dev mailing list -- yt-dev@python.orgmailto:yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.orgmailto:yt-dev-leave@python.org
participants (5)
-
Coughlin, Jared W -
Madicken Munk -
Matthew Turk -
Nathan -
Nathan Goldbaum