I thought that thread was closed already :) I would prefer that we just stick to the original question: *"I think we should gradually switch to using asserts in our tests. WDYT?" * rather than expand it and add more general questions (they are discussed in a separate thread).
>From what I understood so far (maybe I was wrong?) - the majority of people who wrote in this thread wanted to use new asserts. >From your comment Kamil - I understand that the biggest problem you have is that there will be cases where new and old style of asserts will coexist (either in project or even worse - in module). You have no problem with python asserts as such. Please correct me if I am wrong - from the above messages and our discussions I got sometimes different understanding of what you think. So maybe I reformulate the question. I understand we all want to introduce the python asserts. We just differ in how to do it. So here are the options: - A) We want to introduce them as big-bang approach when there is a good time for it (maybe after 2.0). - B) Developer always uses python asserts but only for new modules (consistent test type for modules. All new modules use python asserts). - C) Developer uses python asserts for new/updated tests even if added to existing modules (*) - D) Developer converts whole module to python asserts while he/she does the usual work (**)(***). (*) It can lead to inconsistent tests within the test module. But it's also most review-friendly - the PRs/commits will not contain unrelated test changes. (**) It's not enforced, but once converted there is no way back with mechanism similar to pylint. We provide script that converts the whole module automatically to help the developer. It's the best way to make sure we are going consistently in "python asserts" direction. (**) I think this is really different to what we have now with pylint. We have the tools to automatically convert such tests and the only code changed by that will be the code of tests. Even if we do cherry-picking, we can run the same conversion automatically, this way it will be easy to cherry-pick even if some of the test changes will be unrelated. I am OK to review such automated changes as part of normal changes. They will not change the code of application, just tests, review of this will be easy. It's not an easy choice. My personal preference is *D) *(if I got to choose only one). But I would love to hear what others think - as usual, we should listen to each other and majority should decide. BTW. I do not believe A) will happen. IMHO it's another way of saying "let's not do it at all". There is never a good time for big-bang change. J. On Wed, Jan 1, 2020 at 8:32 PM Kamil Breguła <kamil.breg...@polidea.com> wrote: > I have an additional question and my reply: > > Do we want to allow the use of new conventions for new tests? > No. I prefer keep one convention in the project. > > Do we want to force the use of new conventions for new tests? > No. I prefer keep one convention in the project. > > Do we want to allow mixed conventions as part of the same test? > Example: https://github.com/apache/airflow/pull/6837 > > No. I prefer keep one convention in the project. > > Do we want to use new assert statement? > Yes. After dropping support for Airflow 2.0. I would prefer to focus on > working on new features for Airflow 2.0 > > Do we want to allow the use of new conventions for new tests? > No. We should keep one project convention. We can easily migrate in the > future using an automatic tools. > > Do we want to allow mixed conventions as part of the same test? > Example: https://github.com/apache/airflow/pull/6837 > > Definitely no. This is too confusing. > > Best regards, > Kamil > > On Wed, Jan 1, 2020 at 7:39 PM Kamil Breguła <kamil.breg...@polidea.com> > wrote: > > > Hello, > > > > As I mentioned earlier, I would prefer to maintain one convention for the > > project. The migration of the unittest assertions to the assert > statement > > can be done automatically, At the same time, it will limit discussions > on > > individual PRs. > > > > I think Tomek supports my opinion: > > https://github.com/apache/airflow/pull/6837#pullrequestreview-336927276 > > > > Other people in the discussion also preferred to refrain from migrating > to > > a new style of assertion. > > > > For me, the discussion about **whether** we want to migrate to a new > style > > has ended, but we have not reached a consensus **when** we want to > migrate > > to a new style of the assertion. > > > > Best regards, > > Kamil > > > > On Thu, Dec 19, 2019 at 3:30 PM Jarek Potiuk <jarek.pot...@polidea.com> > > wrote: > > > >> Done. Closing the thread for now. > >> > >> On Wed, Dec 18, 2019 at 6:21 PM Jarek Potiuk <jarek.pot...@polidea.com> > >> wrote: > >> > >> > Fully Agree ! I will do it then if no-one objects! > >> > > >> > On Wed, Dec 18, 2019 at 5:39 PM Tomasz Urbaszek < > >> > tomasz.urbas...@polidea.com> wrote: > >> > > >> >> I agree that global implementation of asserts should happen after > 2.0. > >> >> But in the meantime it seems reasonable to decided on using asserts > in > >> the > >> >> future and enforce that all new, not-conflicting tests should be > >> written in > >> >> pytest way, thus eliminating number of future changes. > >> >> > >> >> Also, after discussion with Kamil I think it would be good to > describe > >> >> the agreed approach in TESTING.md. > >> >> > >> >> T. > >> >> > >> >> On 2019/12/18 16:04:30, Jarek Potiuk <jarek.pot...@polidea.com> > wrote: > >> >> > > > >> >> > > > >> >> > > but I'm not > >> >> > > sure which option is the best. Should we use pytest or unittest > >> >> method? > >> >> > > > >> >> > > >> >> > Yeah. This is the question that started the whole thread. So far > >> only > >> >> > enthusiastic "yes" for the pythonic "asserts" as far as I can see. > >> >> > > >> >> > Anyone has some preference for the unittest ones ? > >> >> > > >> >> > > >> >> > > > >> >> > > On Wed, Dec 18, 2019 at 2:51 PM Kaxil Naik <kaxiln...@gmail.com> > >> >> wrote: > >> >> > > > >> >> > > > Yes I think I agree with that. We should now focus on new > >> features > >> >> and > >> >> > > > improvements for the Users, now that we have done enough to > >> improve > >> >> it > >> >> > > for > >> >> > > > developers. > >> >> > > > > >> >> > > > > >> >> > > > > >> >> > > > On Wed, Dec 18, 2019 at 1:34 PM Kamil Breguła < > >> >> kamil.breg...@polidea.com > >> >> > > > > >> >> > > > wrote: > >> >> > > > > >> >> > > > > Hello, > >> >> > > > > > >> >> > > > > > >> >> > > > > "I'd like to hold off on something as large-scale as > >> >> unittest2pytest > >> >> > > > until > >> >> > > > > 2.0 is out/until we no longer have to backport changes to the > >> >> v1-10-* > >> >> > > > > branches." ~Ash Berlin-Taylor > >> >> > > > > > >> >> > > > > I would also prefer to focus on working on new features for > >> >> Airflow 2.0 > >> >> > > > > instead of improving the developer environment. We already > >> have a > >> >> lot > >> >> > > of > >> >> > > > > changes in the environment, and the code is more solid. > Users > >> >> also > >> >> > > > expect > >> >> > > > > new features so that we don't lag in comparison to other > >> >> products, e.g. > >> >> > > > > Conductor, Prefetch, Dagster. > >> >> > > > > > >> >> > > > > Best regards, > >> >> > > > > Kamil > >> >> > > > > > >> >> > > > > On Thu, Dec 12, 2019 at 7:58 PM Daniel Huang < > >> dxhu...@gmail.com> > >> >> > > wrote: > >> >> > > > > > >> >> > > > > > Big yes to asserts in pytest! > >> >> > > > > > > >> >> > > > > > On Tue, Dec 10, 2019 at 12:50 AM Michał Słowikowski < > >> >> > > > > > michal.slowikow...@polidea.com> wrote: > >> >> > > > > > > >> >> > > > > > > I like this idea, and such assertion looks very > promising. > >> >> > > > > > > > >> >> > > > > > > assert "maximum recursion" in str(excinfo.value) > >> >> > > > > > > > >> >> > > > > > > Have a nice day! > >> >> > > > > > > > >> >> > > > > > > > >> >> > > > > > > On Mon, Dec 9, 2019 at 6:16 PM Tomasz Urbaszek < > >> >> > > > > > > tomasz.urbas...@polidea.com> > >> >> > > > > > > wrote: > >> >> > > > > > > > >> >> > > > > > > > It's definitely worth a try! I even mentioned this in > >> >> AIP-27. > >> >> > > But I > >> >> > > > > > think > >> >> > > > > > > > this script does not change SetUp / TearDown to an > >> >> appropriate > >> >> > > > > > fixture. > >> >> > > > > > > > > >> >> > > > > > > > T. > >> >> > > > > > > > > >> >> > > > > > > > On Mon, Dec 9, 2019 at 6:03 PM Jarek Potiuk < > >> >> > > > > jarek.pot...@polidea.com> > >> >> > > > > > > > wrote: > >> >> > > > > > > > > >> >> > > > > > > > > Well. I think (as discussed with Kamil) maybe even > >> >> convert the > >> >> > > > > tests > >> >> > > > > > > > > automatically. > >> >> > > > > > > > > > >> >> > > > > > > > > How about we try: > >> >> > > https://github.com/pytest-dev/unittest2pytest > >> >> > > > > > > > > > >> >> > > > > > > > > J. > >> >> > > > > > > > > > >> >> > > > > > > > > On Mon, Dec 9, 2019 at 5:48 PM Tomasz Urbaszek < > >> >> > > > > > > > > tomasz.urbas...@polidea.com> wrote: > >> >> > > > > > > > > > >> >> > > > > > > > >> I agree. Should we encourage people to use asserts > >> when > >> >> adding > >> >> > > > new > >> >> > > > > > > > tests? > >> >> > > > > > > > >> > >> >> > > > > > > > >> T. > >> >> > > > > > > > >> > >> >> > > > > > > > >> On Mon, Dec 9, 2019 at 4:35 PM Kaxil Naik < > >> >> > > kaxiln...@gmail.com> > >> >> > > > > > > wrote: > >> >> > > > > > > > >> > >> >> > > > > > > > >>> Yup, we should. > >> >> > > > > > > > >>> > >> >> > > > > > > > >>> On Mon, Dec 9, 2019 at 3:20 PM Ash Berlin-Taylor < > >> >> > > > a...@apache.org > >> >> > > > > > > >> >> > > > > > > > wrote: > >> >> > > > > > > > >>> > >> >> > > > > > > > >>> > Absolutely, and I don't think any of the concers > >> >> about it > >> >> > > in > >> >> > > > > > > run-time > >> >> > > > > > > > >>> code > >> >> > > > > > > > >>> > apply! Plus it is the way pytest recommends, and > I > >> >> think we > >> >> > > > get > >> >> > > > > > > nicer > >> >> > > > > > > > >>> > failure messages using assert-style too? > >> >> > > > > > > > >>> > > >> >> > > > > > > > >>> > -a > >> >> > > > > > > > >>> > > >> >> > > > > > > > >>> > On 9 December 2019 15:06:07 GMT, Jarek Potiuk < > >> >> > > > > > > > >>> jarek.pot...@polidea.com> > >> >> > > > > > > > >>> > wrote: > >> >> > > > > > > > >>> > >Hello everyone. > >> >> > > > > > > > >>> > > > >> >> > > > > > > > >>> > >So asserts are now banned from our main code. > >> >> However with > >> >> > > > the > >> >> > > > > > > > recent > >> >> > > > > > > > >>> > >introduction of pytest we now have a chance to > >> >> switch to > >> >> > > > using > >> >> > > > > > the > >> >> > > > > > > > >>> > >standard > >> >> > > > > > > > >>> > >asserts instead of deriving from TestCase class > >> and > >> >> using > >> >> > > > > > > > >>> > >assertSomething() > >> >> > > > > > > > >>> > >methods. > >> >> > > > > > > > >>> > > > >> >> > > > > > > > >>> > >I find it much more readable and nice and pytest > >> is > >> >> great > >> >> > > in > >> >> > > > > > > > reporting > >> >> > > > > > > > >>> > >the > >> >> > > > > > > > >>> > >errors in a clear and readable way. And all the > >> cases > >> >> > > where > >> >> > > > > > > asserts > >> >> > > > > > > > >>> are > >> >> > > > > > > > >>> > >optimized away are not valid in this case. > >> >> > > > > > > > >>> > > > >> >> > > > > > > > >>> > >I think we should gradually switch to using > >> asserts > >> >> in our > >> >> > > > > > tests. > >> >> > > > > > > > >>> > > > >> >> > > > > > > > >>> > >WDYT? > >> >> > > > > > > > >>> > > > >> >> > > > > > > > >>> > >More info: > >> >> > > > > > > > >>> > > > >> >> > > > > > > > >>> > >Doc about asserts in pytest: > >> >> > > > > > > > >>> > >http://doc.pytest.org/en/latest/assert.html > >> >> > > > > > > > >>> > > > >> >> > > > > > > > >>> > >Demo of common assertion errors produced with > >> pytest: > >> >> > > > > > > > >>> > > > >> >> > > > > > > > >>> > >> >> > > > > > > > > >> >> > > > > > > >> >> > > > > >> >> > >> http://doc.pytest.org/en/latest/example/reportingdemo.html#tbreportdemo > >> >> > > > > > > > >>> > > > >> >> > > > > > > > >>> > >J. > >> >> > > > > > > > >>> > > > >> >> > > > > > > > >>> > >-- > >> >> > > > > > > > >>> > > > >> >> > > > > > > > >>> > >Jarek Potiuk > >> >> > > > > > > > >>> > >Polidea <https://www.polidea.com/> | Principal > >> >> Software > >> >> > > > > > Engineer > >> >> > > > > > > > >>> > > > >> >> > > > > > > > >>> > >M: +48 660 796 129 <+48660796129> > >> >> > > > > > > > >>> > >[image: Polidea] <https://www.polidea.com/> > >> >> > > > > > > > >>> > > >> >> > > > > > > > >>> > >> >> > > > > > > > >> > >> >> > > > > > > > >> > >> >> > > > > > > > >> -- > >> >> > > > > > > > >> > >> >> > > > > > > > >> Tomasz Urbaszek > >> >> > > > > > > > >> Polidea <https://www.polidea.com/> | Junior > Software > >> >> Engineer > >> >> > > > > > > > >> > >> >> > > > > > > > >> M: +48 505 628 493 <+48505628493> > >> >> > > > > > > > >> E: tomasz.urbas...@polidea.com < > >> >> tomasz.urbasz...@polidea.com> > >> >> > > > > > > > >> > >> >> > > > > > > > >> Unique Tech > >> >> > > > > > > > >> Check out our projects! < > >> >> https://www.polidea.com/our-work> > >> >> > > > > > > > >> > >> >> > > > > > > > > > >> >> > > > > > > > > > >> >> > > > > > > > > -- > >> >> > > > > > > > > > >> >> > > > > > > > > Jarek Potiuk > >> >> > > > > > > > > Polidea <https://www.polidea.com/> | Principal > >> Software > >> >> > > Engineer > >> >> > > > > > > > > > >> >> > > > > > > > > M: +48 660 796 129 <+48660796129> > >> >> > > > > > > > > [image: Polidea] <https://www.polidea.com/> > >> >> > > > > > > > > > >> >> > > > > > > > > > >> >> > > > > > > > > >> >> > > > > > > > -- > >> >> > > > > > > > > >> >> > > > > > > > Tomasz Urbaszek > >> >> > > > > > > > Polidea <https://www.polidea.com/> | Junior Software > >> >> Engineer > >> >> > > > > > > > > >> >> > > > > > > > M: +48 505 628 493 <+48505628493> > >> >> > > > > > > > E: tomasz.urbas...@polidea.com < > >> >> tomasz.urbasz...@polidea.com> > >> >> > > > > > > > > >> >> > > > > > > > Unique Tech > >> >> > > > > > > > Check out our projects! < > >> https://www.polidea.com/our-work> > >> >> > > > > > > > > >> >> > > > > > > > >> >> > > > > > > > >> >> > > > > > > -- > >> >> > > > > > > > >> >> > > > > > > Michał Słowikowski > >> >> > > > > > > Polidea <https://www.polidea.com/> | Test Engineer > >> >> > > > > > > > >> >> > > > > > > E: michal.slowikow...@polidea.com > >> >> > > > > > > > >> >> > > > > > > Unique Tech > >> >> > > > > > > Check out our projects! < > https://www.polidea.com/our-work> > >> >> > > > > > > > >> >> > > > > > > >> >> > > > > > >> >> > > > > >> >> > > > >> >> > > >> >> > > >> >> > -- > >> >> > > >> >> > Jarek Potiuk > >> >> > Polidea <https://www.polidea.com/> | Principal Software Engineer > >> >> > > >> >> > M: +48 660 796 129 <+48660796129> > >> >> > [image: Polidea] <https://www.polidea.com/> > >> >> > > >> >> > >> > > >> > > >> > -- > >> > > >> > Jarek Potiuk > >> > Polidea <https://www.polidea.com/> | Principal Software Engineer > >> > > >> > M: +48 660 796 129 <+48660796129> > >> > [image: Polidea] <https://www.polidea.com/> > >> > > >> > > >> > >> -- > >> > >> Jarek Potiuk > >> Polidea <https://www.polidea.com/> | Principal Software Engineer > >> > >> M: +48 660 796 129 <+48660796129> > >> [image: Polidea] <https://www.polidea.com/> > >> > > > -- Jarek Potiuk Polidea <https://www.polidea.com/> | Principal Software Engineer M: +48 660 796 129 <+48660796129> [image: Polidea] <https://www.polidea.com/>
