You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@airflow.apache.org by Tomasz Urbaszek <tu...@apache.org> on 2020/10/22 10:45:32 UTC

Re: Test style convention after migration to pytest

Hi all,

I would like to resurrect this discussion as another one was started in https://github.com/apache/airflow/issues/11657#issuecomment-713834942

Main questions were:

> 1) Do we want to use additional plugins when there is no function in
> the standard library e.g. flaky marker, forked marker?
> 2) Do we want to use custom markers? 
> 3) Do we want to use new assert statement?
> 4) Do we want to remove inheritance from unittest.TestCase?
> 5) Do we want to use class-less tests?
> 6) Do we want to use pytest function instead of current?
> 7) Do we want to use monkeypatch fixture?
> 8) Do we want to use the pytest fixtures?

Probably any changes we decide on should be enforced after 2.0. 

Cheers,
Tomek

On 2020/01/18 01:08:29, Kamil Breguła <ka...@polidea.com> wrote: 
> Hello,
> 
> Sorry for the late reply. Recently, I have been very busy with the
> client project and work on the AIP-21.
> 
> Let's start. I split this message into several sections for easier reading.
> 
> **Establishment of a convention**
> I will prepare a summary. It is available here:
> 
> https://docs.google.com/spreadsheets/d/10qToeTFMxjShBX3xcK6IWr9Sp8Pw7qk35fQTtzpZYYs/edit?usp=sharing
> 
> The mailing list does not allow rich content, so I copy the verdicts.
> 
> 1) Do we want to use additional plugins when there is no function in
> the standard library e.g. flaky marker, forked marker?YES
> 2) Do we want to use custom markers? YES
> 3) Do we want to use new assert statement? YES
> 4) Do we want to remove inheritance from unittest.TestCase? YES
> 5) Do we want to use class-less tests? NO
> 6) Do we want to use pytest function instead of current? No verdict
> 7) Do we want to use monkeypatch fixture? NO
> 8) Do we want to use the pytest fixtures? YES
> 
> We don't have a verdict in question 6, because we have no answer from
> Tomek and Jarek. Can you answer this question? A detailed explanation
> is available.
> https://lists.apache.org/thread.html/7ae7ba0e72d3f0d12f8398a85980c5064b58574caa727c6a974fc628%40%3Cdev.airflow.apache.org%3E
> Should we use the pytest functions or use the standard functions?
> 
> **Using the new convention in the current code**
> tl;dr; Two conventions will be accepted simultaneously.
> Long story: As for the time of using the new convention, I'm the only
> one who is worried about whether to allow it now. Other people did not
> share my fears. So I think we should allow the use of the new
> conventions now. We want to automate the introduction of the new
> convention in the future, so I think that the application of the two
> conventions should be allowed temporarily. However, I will describe
> the new conventions, but we do not need to strictly require them until
> the remaining code is migrated.
> 
> **Full migration to the new convention**
> tl;dr; We'll do it later.
> Long story: Should we migrate the remaining code now?  Tomek prefers
> to wait for the end of work on AIP-21. Kaxil and I also prefer to wait
> for AIP-21 or AIrflow 2.0. Jarek prefers to wait until we finish all
> other works that improve the development environment e.g. pylint. So
> we everybody prefers to wait. I think we can go back to the discussion
> when the pylint is completed or we finish AIP-21. It depends on which
> will be earlier.
> 
> Best regards,
> Kamil
> 
> On Fri, Jan 17, 2020 at 3:07 PM Jarek Potiuk <Ja...@polidea.com> wrote:
> >
> > Should we resume this :)?
> >
> > On Mon, Jan 6, 2020 at 3:42 PM Jarek Potiuk <Ja...@polidea.com>
> > wrote:
> >
> > > Good idea!
> > >
> > > On Mon, Jan 6, 2020 at 3:12 PM Kamil Breguła <ka...@polidea.com>
> > > wrote:
> > >
> > >> Hello,
> > >>
> > >> Now is the holiday period. Some people have not started working yet.
> > >> Others are busy with New Year activities. What do you think about
> > >> Friday?
> > >>
> > >> Best regards,
> > >> Kamil
> > >>
> > >> On Mon, Jan 6, 2020 at 2:48 PM Tomasz Urbaszek
> > >> <to...@polidea.com> wrote:
> > >> >
> > >> > When should we assume that we've reached a consensus?
> > >> >
> > >> > T.
> > >> >
> > >> > On Thu, Jan 2, 2020 at 12:52 AM Jarek Potiuk <Ja...@polidea.com>
> > >> > wrote:
> > >> >
> > >> > > > 1) Do we want to use additional plugins? *Yes*
> > >> > >
> > >> > > 2) Do we want to use custom markers? *Yes. They will help with
> > >> optimising
> > >> > > > our test execution.*
> > >> > >
> > >> > > 3) Do we want to use new assert statement? *Yes*
> > >> > > > 4) Do we want to remove the inheritance from unittest.TestCase? *No
> > >> > > > opinion about it. I am ok with both.*
> > >> > > > 5) Do we want to use class-less tests? *No.*
> > >> > > > 6) Do we want to use pytest function instead of current? *I don't
> > >> > > > understand. Can you explain please?*
> > >> > > > 7) Do we want to use monkeypatch fixture? *No. Mock is better.*
> > >> > > > 8) Do we want to use the pytest fixtures? *Yes.*
> > >> > > >
> > >> > >
> > >> > > Great that we have this discussion now. I also think we should just
> > >> agree
> > >> > > it now and not introduce it "globally".
> > >> > > Once we do it, we should simply add it together new features we
> > >> implement.
> > >> > > We have still pylint to finish as a "non-functional global change"
> > >> and we
> > >> > > should not add new one. It's good to continually improve but one
> > >> thing at a
> > >> > > time.
> > >> > >
> > >> > > BTW Pylint goes well we are down to 243 non-pylint files from 991
> > >> since
> > >> > > May.
> > >> > >
> > >> > > J.
> > >> > >
> > >> > > On Thu, Jan 2, 2020 at 12:40 AM Kaxil Naik <ka...@gmail.com>
> > >> wrote:
> > >> > >
> > >> > > > This is a tough one. Both arguments are reasonable.
> > >> > > >
> > >> > > > I agree at some point we should convert all to use assert. But at
> > >> the
> > >> > > same
> > >> > > > time, we should also focus on adding *more user-facing features *and
> > >> > > spend
> > >> > > > less time on more refactor or similar changes.
> > >> > > >
> > >> > > > So based on that, this might be a low priority. We also need to
> > >> still
> > >> > > > complete AIP-21
> > >> > > > <
> > >> > > >
> > >> > >
> > >> https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-21%3A+Changes+in+import+paths
> > >> > > > >
> > >> > > > which is very critical for 2.0.
> > >> > > >
> > >> > > > 1) Do we want to use additional plugins?
> > >> > > >
> > >> > > >
> > >> > > > Yes.
> > >> > > >
> > >> > > > 2) Do we want to use custom markers?
> > >> > > >
> > >> > > >
> > >> > > > Not sure yet. Low Priority for me.
> > >> > > >
> > >> > > > 3) Do we want to use new assert statement?
> > >> > > >
> > >> > > >
> > >> > > > I think new PRs can contain it, shouldn't be a problem as long as
> > >> it is
> > >> > > > documented to avoid confusion.
> > >> > > >
> > >> > > > 4) Do we want to remove the inheritance from unittest.TestCase?
> > >> > > >
> > >> > > >
> > >> > > > Yes, this is, however, going to change how people write tests. So
> > >> someone
> > >> > > > has to own it as it can become painful with PRs getting merged
> > >> > > > continuously.
> > >> > > >
> > >> > > > 5) Do we want to use class-less tests?
> > >> > > >
> > >> > > >
> > >> > > > No.
> > >> > > >
> > >> > > > 6) Do we want to use pytest function instead of current?
> > >> > > >
> > >> > > >
> > >> > > > Yes
> > >> > > >
> > >> > > > 7) Do we want to use monkeypatch fixture?
> > >> > > >
> > >> > > >
> > >> > > > I also prefer unittest.mock but open to suggestions.
> > >> > > >
> > >> https://github.com/pytest-dev/pytest/issues/4576#issuecomment-449864333
> > >> > > > has
> > >> > > > some good comparison on it
> > >> > > >
> > >> > > > 8) Do we want to use the pytest fixtures?
> > >> > > >
> > >> > > >
> > >> > > > Yes
> > >> > > >
> > >> > > > On Wed, Jan 1, 2020 at 8:07 PM Kamil Breguła <
> > >> kamil.bregula@polidea.com>
> > >> > > > wrote:
> > >> > > >
> > >> > > > >     @unittest.skip("demonstrating skipping")
> > >> > > > >
> > >> > > > > On Wed, Jan 1, 2020 at 8:37 PM Tomasz Urbaszek <
> > >> turbaszek@apache.org>
> > >> > > > > wrote:
> > >> > > > > >
> > >> > > > > >
> > >> > > > > > > 6) Do we want to use pytest function instead of current?
> > >> > > > > >
> > >> > > > > > I do not understand this point, can you explain?
> > >> > > > > >
> > >> > > > >
> > >> > > > > Pytest Introduces solutions that replace solutions that are now
> > >> used
> > >> > > > >
> > >> > > > > For example:
> > >> > > > > def test_foo(self):
> > >> > > > >     wtih self.assertRaises(AirflowException):
> > >> > > > >            bar()
> > >> > > > >
> > >> > > > > Can be replaced by following code:
> > >> > > > >
> > >> > > > > from pytest import raises
> > >> > > > >
> > >> > > > > wtih raises(AirflowException):
> > >> > > > >     bar()
> > >> > > > >
> > >> > > > > OR
> > >> > > > >
> > >> > > > > from parametrize import parametrize
> > >> > > > >
> > >> > > > > @parametrize.expand([
> > >> > > > >     (1, 1, ),
> > >> > > > >     (2, 2, ),
> > >> > > > > ])
> > >> > > > > def test_foo(self, param_a, param_b);
> > >> > > > >     self.assertEqual(param_a, param_b)
> > >> > > > >
> > >> > > > > can be replaced by
> > >> > > > >
> > >> > > > > @pytest.mark.parametrize("param_a,param_b", [(1, 1), (2, 2),])
> > >> > > > > def test_eval(param_a, param_b):
> > >> > > > >     assert param_a == param_b
> > >> > > > >
> > >> > > > > OR
> > >> > > > >
> > >> > > > > @unittest.skip("demonstrating skipping")
> > >> > > > > def test_foo(self)
> > >> > > > >
> > >> > > > > can be replaced by
> > >> > > > >
> > >> > > > > @pytest.mark.skip(reason="demonstrating skipping"))
> > >> > > > > def test_foo(self)
> > >> > > > >
> > >> > > > > Which solution will be better for us?
> > >> > > > >
> > >> > > > > >
> > >> > > > > > On Wed, Jan 1, 2020 at 8:14 PM Kamil Breguła <
> > >> > > > kamil.bregula@polidea.com>
> > >> > > > > > wrote:
> > >> > > > > >
> > >> > > > > > > > 1) Do we want to use additional plugins?
> > >> > > > > > >
> > >> > > > > > > Yes. We should use the full-power of plugins now.
> > >> > > > > > >
> > >> > > > > > > > 2) Do we want to use custom markers?
> > >> > > > > > >
> > >> > > > > > > Reply in a separate thread.
> > >> > > > > > >
> > >> > > > > > > >3) Do we want to use new assert statement?
> > >> > > > > > >
> > >> > > > > > > Reply in a separate thread
> > >> > > > > > >
> > >> > > > > > > > 4) Do we want to remove the inheritance from
> > >> unittest.TestCase?
> > >> > > > > > >
> > >> > > > > > > Yes. After dropping support for Airflow 2.0, if possible. I
> > >> would
> > >> > > > > prefer to
> > >> > > > > > > focus on working on new features for Airflow 2.0.
> > >> > > > > > >
> > >> > > > > > > > 5) Do we want to use class-less tests?
> > >> > > > > > >
> > >> > > > > > > No. Classes allow easy grouping of tests. Even if a file with
> > >> one
> > >> > > > > class now
> > >> > > > > > > exists, a new one may appear in the future.
> > >> > > > > > >
> > >> > > > > > > > 6) Do we want to use pytest function instead of current?
> > >> > > > > > >
> > >> > > > > > > I feel good about the current functions. However, this is not
> > >> a
> > >> > > > serious
> > >> > > > > > > relationship and I can create a new friendship.
> > >> > > > > > >
> > >> > > > > > > > 7) Do we want to use monkeypatch fixture?
> > >> > > > > > >
> > >> > > > > > > No. I prefer unittest.mock
> > >> > > > > > >
> > >> > > > > > > > 8) Do we want to use the pytest fixtures?
> > >> > > > > > >
> > >> > > > > > > No. I prefer classic fixtures, if possible. Their syntax is
> > >> much
> > >> > > > > simpler
> > >> > > > > > > and easier to understand.
> > >> > > > > > >
> > >> > > > > > > On Wed, Jan 1, 2020 at 8:10 PM Kamil Breguła <
> > >> > > > > kamil.bregula@polidea.com>
> > >> > > > > > > wrote:
> > >> > > > > > >
> > >> > > > > > > > Hello,
> > >> > > > > > > >
> > >> > > > > > > >
> > >> > > > > > > > We have recently migrated to pytest.  Code written
> > >> according to
> > >> > > the
> > >> > > > > > > > standard library - unittest.TestCase is still compatible
> > >> with
> > >> > > > pytest.
> > >> > > > > > > >
> > >> > > > > > > >
> > >> > > > > > > > The AIP-21 document did not discuss the migration of
> > >> current code
> > >> > > > to
> > >> > > > > > > > new features, but only discussed the change of runner and
> > >> > > benefits
> > >> > > > of
> > >> > > > > > > > pytest over nosetets.
> > >> > > > > > > >
> > >> > > > > > > > Link:
> > >> > > > > > > >
> > >> > > > > > >
> > >> > > > >
> > >> > > >
> > >> > >
> > >> https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-27+Migrate+to+pytest
> > >> > > > > > > >
> > >> > > > > > > >
> > >> > > > > > > > As far as I appreciate the many advantages of using this
> > >> tool,
> > >> > > but
> > >> > > > I
> > >> > > > > am
> > >> > > > > > > > not sure **whether, how or when we want to start using some
> > >> > > > > features**. I
> > >> > > > > > > > prefer to keep the current project conventions in many
> > >> areas,
> > >> > > but I
> > >> > > > > still
> > >> > > > > > > > love pytest. I think we should establish general
> > >> conventions on
> > >> > > > > writing
> > >> > > > > > > > tests and recommended solutions to known problems. I prefer
> > >> a
> > >> > > > > pragmatic
> > >> > > > > > > > approach, not just the use of features, because now we can
> > >> use
> > >> > > it.
> > >> > > > > > > > Unfortunately, I do not see many benefits from new features.
> > >> > > > > > > >
> > >> > > > > > > >
> > >> > > > > > > > I would not like the code to have many ways of expressing
> > >> the
> > >> > > same
> > >> > > > > > > > solution. For the following reasons:
> > >> > > > > > > >
> > >> > > > > > > >
> > >> > > > > > > > * it can introduce a lack of understanding among new
> > >> contributors
> > >> > > > > > > >
> > >> > > > > > > > * facilitate the understanding and reading of code.
> > >> > > > > > > >
> > >> > > > > > > > * creates unnecessary discussion about the preferences of
> > >> one way
> > >> > > > > over
> > >> > > > > > > the
> > >> > > > > > > > other. Not related to changes.
> > >> > > > > > > >
> > >> > > > > > > > * forces an understanding of the complex syntax of some
> > >> > > solutions.
> > >> > > > > > > >
> > >> > > > > > > > * encourages people to rewrite the code, which can generate
> > >> > > > > unnecessary
> > >> > > > > > > > work.
> > >> > > > > > > >
> > >> > > > > > > >
> > >> > > > > > > > To establish a full convention, I have prepared a few
> > >> questions:
> > >> > > > > > > >
> > >> > > > > > > > 1) Do we want to use additional plugins when there is no
> > >> function
> > >> > > > in
> > >> > > > > the
> > >> > > > > > > > standard library e.g. flaky marker, forked marker?  This
> > >> is, in
> > >> > > my
> > >> > > > > > > opinion,
> > >> > > > > > > > one of the big advantages of migrating to pytest.
> > >> > > > > > > >
> > >> > > > > > > > 2) Do we want to use custom markers?
> > >> > > > > > > >
> > >> > > > > > > > The discussion takes place in a separate thread:
> > >> > > > > > > >
> > >> > > > > > > >
> > >> > > > > > > >
> > >> > > > > > >
> > >> > > > >
> > >> > > >
> > >> > >
> > >> https://lists.apache.org/thread.html/4538437c96f599766005ba7829d0bee1511debb4f53599e0d300a56f%40%3Cdev.airflow.apache.org%3E
> > >> > > > > > > >
> > >> > > > > > > > 3) Do we want to use new assert statement?
> > >> > > > > > > >
> > >> > > > > > > >
> > >> > > > > > > >
> > >> > > > > > >
> > >> > > > >
> > >> > > >
> > >> > >
> > >> https://lists.apache.org/thread.html/08b64d3b084c865399f98f6c6f56235ce5329e843d97938e1a8045a5%40%3Cdev.airflow.apache.org%3E
> > >> > > > > > > >
> > >> > > > > > > > Based on the discussion with devlist, we want to delay
> > >> migrations
> > >> > > > to
> > >> > > > > the
> > >> > > > > > > > new assert statement.
> > >> > > > > > > >
> > >> > > > > > > > 4) Do we want to remove inheritance from unittest.TestCase?
> > >> > > > > > > >
> > >> > > > > > > > 5) Do we want to use class-less tests?
> > >> > > > > > > >
> > >> > > > > > > > 6) Do we want to use pytest function instead of current?
> > >> > > > > > > >
> > >> > > > > > > > For example:
> > >> > > > > > > >
> > >> > > > > > > > Unittest.assertRaises vs pytest.raises
> > >> > > > > > > >
> > >> > > > > > > > parametrize vs pytest.mark.parametrize
> > >> > > > > > > >
> > >> > > > > > > > unittest.skip[If], vs  pytest.mark.skip[If]
> > >> > > > > > > >
> > >> > > > > > > > 7) Do we want to use monkeypatch fixture?
> > >> > > > > > > >
> > >> > > > > > > > https://docs.pytest.org/en/latest/monkeypatch.html
> > >> > > > > > > >
> > >> > > > > > > > 8) Do we want to use the pytest fixtures?
> > >> > > > > > > >
> > >> > > > > > > > Do we want to migrate all code to pytest fixtures?
> > >> > > > > > > >
> > >> > > > > > > > We are currently using a different style of fixtures. Do we
> > >> want
> > >> > > to
> > >> > > > > give
> > >> > > > > > > > it up?
> > >> > > > > > > >
> > >> > > > > > > >
> > >> > > > > > >
> > >> > > > >
> > >> > > >
> > >> > >
> > >> https://docs.python.org/3/library/unittest.html#class-and-module-fixtures
> > >> > > > > > > >
> > >> > > > > > > >
> > >> > > > > > > > It is worth asking to think about whether we do not want to
> > >> > > change
> > >> > > > > this
> > >> > > > > > > > convention in the future e.g. after dropping support for
> > >> 1.10.X.
> > >> > > We
> > >> > > > > can
> > >> > > > > > > > also allow two conventions on a temporary basis, and then
> > >> migrate
> > >> > > > to
> > >> > > > > one
> > >> > > > > > > at
> > >> > > > > > > > a later time. For example, we want to migrate to the assert
> > >> > > > statement
> > >> > > > > > > after
> > >> > > > > > > > dropping support for 1.10
> > >> > > > > > > >
> > >> > > > > > > >
> > >> > > > > > > > I hope I found the main differences between the current
> > >> > > convention
> > >> > > > > and
> > >> > > > > > > the
> > >> > > > > > > > new convention. However, if you missed something, please
> > >> continue
> > >> > > > to
> > >> > > > > > > number.
> > >> > > > > > > >
> > >> > > > > > > >
> > >> > > > > > > > Best regards,
> > >> > > > > > > >
> > >> > > > > > > > Kamil
> > >> > > > > > > >
> > >> > > > > > > >
> > >> > > > > > > >
> > >> > > > > > > >
> > >> > > > > > >
> > >> > > > >
> > >> > > >
> > >> > >
> > >> > >
> > >> > > --
> > >> > >
> > >> > > 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/> | Software Engineer
> > >> >
> > >> > M: +48 505 628 493 <+48505628493>
> > >> > E: tomasz.urbaszek@polidea.com <to...@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/>
>

Re: Test style convention after migration to pytest

Posted by Ash Berlin-Taylor <as...@apache.org>.

👍🏻

On 10 January 2021 12:18:28 GMT, Tomasz Urbaszek <tu...@apache.org> wrote:
>Hello all!
>
>The PR for changing self.assert to plain assert is here:
>https://github.com/apache/airflow/pull/12951
>
>All tests seem to be passing (except on heisentest) so I'm going to merge
>on Monday if there are no objections.
>
>Cheers,
>Tomek
>
>On Thu, Oct 22, 2020 at 1:04 PM Ash Berlin-Taylor <as...@apache.org> wrote:
>
>> 2. We already do don't we for backend and integrations etc?
>> 3. I am strongly in favour of -- using assert  with pytest gives much
>> nicer error messages than unittest's own assertions;
>> 4. if we do 3, then I'm also in favour of doing 4 as we don't need it
>> anymore, and we can then use more plugins/features of pytest (parameterized
>> behaves better I think)
>> 5. No view for or against
>> 6. depends on what we do for 4
>> 7. unittest.mock can be confusing to use, but I'm not sure pytests
>> monkeypatch fixture is nicer. No opinoin on this one
>> 8. I like them, but the automatic dependency injection can be confusing
>> until you are familar with Pytest. An example of them for anyone unaware:
>> https://github.com/apache/airflow/pull/11694 (look at the example test at
>> the end of the PR description)
>>
>> I don't think we need to (yet) do any bulk changes to the test, but if we
>> agree that using more of pytest is the way to go then we can write new
>> tests using assert style, and once we've got a good chunk of the way we can
>> bulk switch the rest.
>>
>> -ash
>>
>> On Oct 22 2020, at 11:45 am, Tomasz Urbaszek <tu...@apache.org> wrote:
>>
>> Hi all,
>>
>> I would like to resurrect this discussion as another one was started in
>> https://github.com/apache/airflow/issues/11657#issuecomment-713834942
>>
>> Main questions were:
>>
>> > 1) Do we want to use additional plugins when there is no function in
>> > the standard library e.g. flaky marker, forked marker?
>> > 2) Do we want to use custom markers?
>> > 3) Do we want to use new assert statement?
>> > 4) Do we want to remove inheritance from unittest.TestCase?
>> > 5) Do we want to use class-less tests?
>> > 6) Do we want to use pytest function instead of current?
>> > 7) Do we want to use monkeypatch fixture?
>> > 8) Do we want to use the pytest fixtures?
>>
>> Probably any changes we decide on should be enforced after 2.0.
>>
>> Cheers,
>> Tomek
>>
>> On 2020/01/18 01:08:29, Kamil Breguła <ka...@polidea.com> wrote:
>> > Hello,
>> >
>> > Sorry for the late reply. Recently, I have been very busy with the
>> > client project and work on the AIP-21.
>> >
>> > Let's start. I split this message into several sections for easier
>> reading.
>> >
>> > **Establishment of a convention**
>> > I will prepare a summary. It is available here:
>> >
>> >
>> https://docs.google.com/spreadsheets/d/10qToeTFMxjShBX3xcK6IWr9Sp8Pw7qk35fQTtzpZYYs/edit?usp=sharing
>> >
>> > The mailing list does not allow rich content, so I copy the verdicts.
>> >
>> > 1) Do we want to use additional plugins when there is no function in
>> > the standard library e.g. flaky marker, forked marker?YES
>> > 2) Do we want to use custom markers? YES
>> > 3) Do we want to use new assert statement? YES
>> > 4) Do we want to remove inheritance from unittest.TestCase? YES
>> > 5) Do we want to use class-less tests? NO
>> > 6) Do we want to use pytest function instead of current? No verdict
>> > 7) Do we want to use monkeypatch fixture? NO
>> > 8) Do we want to use the pytest fixtures? YES
>> >
>> > We don't have a verdict in question 6, because we have no answer from
>> > Tomek and Jarek. Can you answer this question? A detailed explanation
>> > is available.
>> >
>> https://lists.apache.org/thread.html/7ae7ba0e72d3f0d12f8398a85980c5064b58574caa727c6a974fc628%40%3Cdev.airflow.apache.org%3E
>> > Should we use the pytest functions or use the standard functions?
>> >
>> > **Using the new convention in the current code**
>> > tl;dr; Two conventions will be accepted simultaneously.
>> > Long story: As for the time of using the new convention, I'm the only
>> > one who is worried about whether to allow it now. Other people did not
>> > share my fears. So I think we should allow the use of the new
>> > conventions now. We want to automate the introduction of the new
>> > convention in the future, so I think that the application of the two
>> > conventions should be allowed temporarily. However, I will describe
>> > the new conventions, but we do not need to strictly require them until
>> > the remaining code is migrated.
>> >
>> > **Full migration to the new convention**
>> > tl;dr; We'll do it later.
>> > Long story: Should we migrate the remaining code now? Tomek prefers
>> > to wait for the end of work on AIP-21. Kaxil and I also prefer to wait
>> > for AIP-21 or AIrflow 2.0. Jarek prefers to wait until we finish all
>> > other works that improve the development environment e.g. pylint. So
>> > we everybody prefers to wait. I think we can go back to the discussion
>> > when the pylint is completed or we finish AIP-21. It depends on which
>> > will be earlier.
>> >
>> > Best regards,
>> > Kamil
>> >
>> > On Fri, Jan 17, 2020 at 3:07 PM Jarek Potiuk <Ja...@polidea.com>
>> wrote:
>> > >
>> > > Should we resume this :)?
>> > >
>> > > On Mon, Jan 6, 2020 at 3:42 PM Jarek Potiuk <Ja...@polidea.com>
>> > > wrote:
>> > >
>> > > > Good idea!
>> > > >
>> > > > On Mon, Jan 6, 2020 at 3:12 PM Kamil Breguła <
>> kamil.bregula@polidea.com>
>> > > > wrote:
>> > > >
>> > > >> Hello,
>> > > >>
>> > > >> Now is the holiday period. Some people have not started working yet.
>> > > >> Others are busy with New Year activities. What do you think about
>> > > >> Friday?
>> > > >>
>> > > >> Best regards,
>> > > >> Kamil
>> > > >>
>> > > >> On Mon, Jan 6, 2020 at 2:48 PM Tomasz Urbaszek
>> > > >> <to...@polidea.com> wrote:
>> > > >> >
>> > > >> > When should we assume that we've reached a consensus?
>> > > >> >
>> > > >> > T.
>> > > >> >
>> > > >> > On Thu, Jan 2, 2020 at 12:52 AM Jarek Potiuk <
>> Jarek.Potiuk@polidea.com>
>> > > >> > wrote:
>> > > >> >
>> > > >> > > > 1) Do we want to use additional plugins? *Yes*
>> > > >> > >
>> > > >> > > 2) Do we want to use custom markers? *Yes. They will help with
>> > > >> optimising
>> > > >> > > > our test execution.*
>> > > >> > >
>> > > >> > > 3) Do we want to use new assert statement? *Yes*
>> > > >> > > > 4) Do we want to remove the inheritance from
>> unittest.TestCase? *No
>> > > >> > > > opinion about it. I am ok with both.*
>> > > >> > > > 5) Do we want to use class-less tests? *No.*
>> > > >> > > > 6) Do we want to use pytest function instead of current? *I
>> don't
>> > > >> > > > understand. Can you explain please?*
>> > > >> > > > 7) Do we want to use monkeypatch fixture? *No. Mock is
>> better.*
>> > > >> > > > 8) Do we want to use the pytest fixtures? *Yes.*
>> > > >> > > >
>> > > >> > >
>> > > >> > > Great that we have this discussion now. I also think we should
>> just
>> > > >> agree
>> > > >> > > it now and not introduce it "globally".
>> > > >> > > Once we do it, we should simply add it together new features we
>> > > >> implement.
>> > > >> > > We have still pylint to finish as a "non-functional global
>> change"
>> > > >> and we
>> > > >> > > should not add new one. It's good to continually improve but one
>> > > >> thing at a
>> > > >> > > time.
>> > > >> > >
>> > > >> > > BTW Pylint goes well we are down to 243 non-pylint files from
>> 991
>> > > >> since
>> > > >> > > May.
>> > > >> > >
>> > > >> > > J.
>> > > >> > >
>> > > >> > > On Thu, Jan 2, 2020 at 12:40 AM Kaxil Naik <kaxilnaik@gmail.com
>> >
>> > > >> wrote:
>> > > >> > >
>> > > >> > > > This is a tough one. Both arguments are reasonable.
>> > > >> > > >
>> > > >> > > > I agree at some point we should convert all to use assert.
>> But at
>> > > >> the
>> > > >> > > same
>> > > >> > > > time, we should also focus on adding *more user-facing
>> features *and
>> > > >> > > spend
>> > > >> > > > less time on more refactor or similar changes.
>> > > >> > > >
>> > > >> > > > So based on that, this might be a low priority. We also need
>> to
>> > > >> still
>> > > >> > > > complete AIP-21
>> > > >> > > > <
>> > > >> > > >
>> > > >> > >
>> > > >>
>> https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-21%3A+Changes+in+import+paths
>> > > >> > > > >
>> > > >> > > > which is very critical for 2.0.
>> > > >> > > >
>> > > >> > > > 1) Do we want to use additional plugins?
>> > > >> > > >
>> > > >> > > >
>> > > >> > > > Yes.
>> > > >> > > >
>> > > >> > > > 2) Do we want to use custom markers?
>> > > >> > > >
>> > > >> > > >
>> > > >> > > > Not sure yet. Low Priority for me.
>> > > >> > > >
>> > > >> > > > 3) Do we want to use new assert statement?
>> > > >> > > >
>> > > >> > > >
>> > > >> > > > I think new PRs can contain it, shouldn't be a problem as
>> long as
>> > > >> it is
>> > > >> > > > documented to avoid confusion.
>> > > >> > > >
>> > > >> > > > 4) Do we want to remove the inheritance from
>> unittest.TestCase?
>> > > >> > > >
>> > > >> > > >
>> > > >> > > > Yes, this is, however, going to change how people write
>> tests. So
>> > > >> someone
>> > > >> > > > has to own it as it can become painful with PRs getting merged
>> > > >> > > > continuously.
>> > > >> > > >
>> > > >> > > > 5) Do we want to use class-less tests?
>> > > >> > > >
>> > > >> > > >
>> > > >> > > > No.
>> > > >> > > >
>> > > >> > > > 6) Do we want to use pytest function instead of current?
>> > > >> > > >
>> > > >> > > >
>> > > >> > > > Yes
>> > > >> > > >
>> > > >> > > > 7) Do we want to use monkeypatch fixture?
>> > > >> > > >
>> > > >> > > >
>> > > >> > > > I also prefer unittest.mock but open to suggestions.
>> > > >> > > >
>> > > >>
>> https://github.com/pytest-dev/pytest/issues/4576#issuecomment-449864333
>> > > >> > > > has
>> > > >> > > > some good comparison on it
>> > > >> > > >
>> > > >> > > > 8) Do we want to use the pytest fixtures?
>> > > >> > > >
>> > > >> > > >
>> > > >> > > > Yes
>> > > >> > > >
>> > > >> > > > On Wed, Jan 1, 2020 at 8:07 PM Kamil Breguła <
>> > > >> kamil.bregula@polidea.com>
>> > > >> > > > wrote:
>> > > >> > > >
>> > > >> > > > > @unittest.skip("demonstrating skipping")
>> > > >> > > > >
>> > > >> > > > > On Wed, Jan 1, 2020 at 8:37 PM Tomasz Urbaszek <
>> > > >> turbaszek@apache.org>
>> > > >> > > > > wrote:
>> > > >> > > > > >
>> > > >> > > > > >
>> > > >> > > > > > > 6) Do we want to use pytest function instead of current?
>> > > >> > > > > >
>> > > >> > > > > > I do not understand this point, can you explain?
>> > > >> > > > > >
>> > > >> > > > >
>> > > >> > > > > Pytest Introduces solutions that replace solutions that are
>> now
>> > > >> used
>> > > >> > > > >
>> > > >> > > > > For example:
>> > > >> > > > > def test_foo(self):
>> > > >> > > > > wtih self.assertRaises(AirflowException):
>> > > >> > > > > bar()
>> > > >> > > > >
>> > > >> > > > > Can be replaced by following code:
>> > > >> > > > >
>> > > >> > > > > from pytest import raises
>> > > >> > > > >
>> > > >> > > > > wtih raises(AirflowException):
>> > > >> > > > > bar()
>> > > >> > > > >
>> > > >> > > > > OR
>> > > >> > > > >
>> > > >> > > > > from parametrize import parametrize
>> > > >> > > > >
>> > > >> > > > > @parametrize.expand([
>> > > >> > > > > (1, 1, ),
>> > > >> > > > > (2, 2, ),
>> > > >> > > > > ])
>> > > >> > > > > def test_foo(self, param_a, param_b);
>> > > >> > > > > self.assertEqual(param_a, param_b)
>> > > >> > > > >
>> > > >> > > > > can be replaced by
>> > > >> > > > >
>> > > >> > > > > @pytest.mark.parametrize("param_a,param_b", [(1, 1), (2,
>> 2),])
>> > > >> > > > > def test_eval(param_a, param_b):
>> > > >> > > > > assert param_a == param_b
>> > > >> > > > >
>> > > >> > > > > OR
>> > > >> > > > >
>> > > >> > > > > @unittest.skip("demonstrating skipping")
>> > > >> > > > > def test_foo(self)
>> > > >> > > > >
>> > > >> > > > > can be replaced by
>> > > >> > > > >
>> > > >> > > > > @pytest.mark.skip(reason="demonstrating skipping"))
>> > > >> > > > > def test_foo(self)
>> > > >> > > > >
>> > > >> > > > > Which solution will be better for us?
>> > > >> > > > >
>> > > >> > > > > >
>> > > >> > > > > > On Wed, Jan 1, 2020 at 8:14 PM Kamil Breguła <
>> > > >> > > > kamil.bregula@polidea.com>
>> > > >> > > > > > wrote:
>> > > >> > > > > >
>> > > >> > > > > > > > 1) Do we want to use additional plugins?
>> > > >> > > > > > >
>> > > >> > > > > > > Yes. We should use the full-power of plugins now.
>> > > >> > > > > > >
>> > > >> > > > > > > > 2) Do we want to use custom markers?
>> > > >> > > > > > >
>> > > >> > > > > > > Reply in a separate thread.
>> > > >> > > > > > >
>> > > >> > > > > > > >3) Do we want to use new assert statement?
>> > > >> > > > > > >
>> > > >> > > > > > > Reply in a separate thread
>> > > >> > > > > > >
>> > > >> > > > > > > > 4) Do we want to remove the inheritance from
>> > > >> unittest.TestCase?
>> > > >> > > > > > >
>> > > >> > > > > > > Yes. After dropping support for Airflow 2.0, if
>> possible. I
>> > > >> would
>> > > >> > > > > prefer to
>> > > >> > > > > > > focus on working on new features for Airflow 2.0.
>> > > >> > > > > > >
>> > > >> > > > > > > > 5) Do we want to use class-less tests?
>> > > >> > > > > > >
>> > > >> > > > > > > No. Classes allow easy grouping of tests. Even if a
>> file with
>> > > >> one
>> > > >> > > > > class now
>> > > >> > > > > > > exists, a new one may appear in the future.
>> > > >> > > > > > >
>> > > >> > > > > > > > 6) Do we want to use pytest function instead of
>> current?
>> > > >> > > > > > >
>> > > >> > > > > > > I feel good about the current functions. However, this
>> is not
>> > > >> a
>> > > >> > > > serious
>> > > >> > > > > > > relationship and I can create a new friendship.
>> > > >> > > > > > >
>> > > >> > > > > > > > 7) Do we want to use monkeypatch fixture?
>> > > >> > > > > > >
>> > > >> > > > > > > No. I prefer unittest.mock
>> > > >> > > > > > >
>> > > >> > > > > > > > 8) Do we want to use the pytest fixtures?
>> > > >> > > > > > >
>> > > >> > > > > > > No. I prefer classic fixtures, if possible. Their
>> syntax is
>> > > >> much
>> > > >> > > > > simpler
>> > > >> > > > > > > and easier to understand.
>> > > >> > > > > > >
>> > > >> > > > > > > On Wed, Jan 1, 2020 at 8:10 PM Kamil Breguła <
>> > > >> > > > > kamil.bregula@polidea.com>
>> > > >> > > > > > > wrote:
>> > > >> > > > > > >
>> > > >> > > > > > > > Hello,
>> > > >> > > > > > > >
>> > > >> > > > > > > >
>> > > >> > > > > > > > We have recently migrated to pytest. Code written
>> > > >> according to
>> > > >> > > the
>> > > >> > > > > > > > standard library - unittest.TestCase is still
>> compatible
>> > > >> with
>> > > >> > > > pytest.
>> > > >> > > > > > > >
>> > > >> > > > > > > >
>> > > >> > > > > > > > The AIP-21 document did not discuss the migration of
>> > > >> current code
>> > > >> > > > to
>> > > >> > > > > > > > new features, but only discussed the change of runner
>> and
>> > > >> > > benefits
>> > > >> > > > of
>> > > >> > > > > > > > pytest over nosetets.
>> > > >> > > > > > > >
>> > > >> > > > > > > > Link:
>> > > >> > > > > > > >
>> > > >> > > > > > >
>> > > >> > > > >
>> > > >> > > >
>> > > >> > >
>> > > >>
>> https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-27+Migrate+to+pytest
>> > > >> > > > > > > >
>> > > >> > > > > > > >
>> > > >> > > > > > > > As far as I appreciate the many advantages of using
>> this
>> > > >> tool,
>> > > >> > > but
>> > > >> > > > I
>> > > >> > > > > am
>> > > >> > > > > > > > not sure **whether, how or when we want to start
>> using some
>> > > >> > > > > features**. I
>> > > >> > > > > > > > prefer to keep the current project conventions in many
>> > > >> areas,
>> > > >> > > but I
>> > > >> > > > > still
>> > > >> > > > > > > > love pytest. I think we should establish general
>> > > >> conventions on
>> > > >> > > > > writing
>> > > >> > > > > > > > tests and recommended solutions to known problems. I
>> prefer
>> > > >> a
>> > > >> > > > > pragmatic
>> > > >> > > > > > > > approach, not just the use of features, because now
>> we can
>> > > >> use
>> > > >> > > it.
>> > > >> > > > > > > > Unfortunately, I do not see many benefits from new
>> features.
>> > > >> > > > > > > >
>> > > >> > > > > > > >
>> > > >> > > > > > > > I would not like the code to have many ways of
>> expressing
>> > > >> the
>> > > >> > > same
>> > > >> > > > > > > > solution. For the following reasons:
>> > > >> > > > > > > >
>> > > >> > > > > > > >
>> > > >> > > > > > > > * it can introduce a lack of understanding among new
>> > > >> contributors
>> > > >> > > > > > > >
>> > > >> > > > > > > > * facilitate the understanding and reading of code.
>> > > >> > > > > > > >
>> > > >> > > > > > > > * creates unnecessary discussion about the
>> preferences of
>> > > >> one way
>> > > >> > > > > over
>> > > >> > > > > > > the
>> > > >> > > > > > > > other. Not related to changes.
>> > > >> > > > > > > >
>> > > >> > > > > > > > * forces an understanding of the complex syntax of
>> some
>> > > >> > > solutions.
>> > > >> > > > > > > >
>> > > >> > > > > > > > * encourages people to rewrite the code, which can
>> generate
>> > > >> > > > > unnecessary
>> > > >> > > > > > > > work.
>> > > >> > > > > > > >
>> > > >> > > > > > > >
>> > > >> > > > > > > > To establish a full convention, I have prepared a few
>> > > >> questions:
>> > > >> > > > > > > >
>> > > >> > > > > > > > 1) Do we want to use additional plugins when there is
>> no
>> > > >> function
>> > > >> > > > in
>> > > >> > > > > the
>> > > >> > > > > > > > standard library e.g. flaky marker, forked marker?
>> This
>> > > >> is, in
>> > > >> > > my
>> > > >> > > > > > > opinion,
>> > > >> > > > > > > > one of the big advantages of migrating to pytest.
>> > > >> > > > > > > >
>> > > >> > > > > > > > 2) Do we want to use custom markers?
>> > > >> > > > > > > >
>> > > >> > > > > > > > The discussion takes place in a separate thread:
>> > > >> > > > > > > >
>> > > >> > > > > > > >
>> > > >> > > > > > > >
>> > > >> > > > > > >
>> > > >> > > > >
>> > > >> > > >
>> > > >> > >
>> > > >>
>> https://lists.apache.org/thread.html/4538437c96f599766005ba7829d0bee1511debb4f53599e0d300a56f%40%3Cdev.airflow.apache.org%3E
>> > > >> > > > > > > >
>> > > >> > > > > > > > 3) Do we want to use new assert statement?
>> > > >> > > > > > > >
>> > > >> > > > > > > >
>> > > >> > > > > > > >
>> > > >> > > > > > >
>> > > >> > > > >
>> > > >> > > >
>> > > >> > >
>> > > >>
>> https://lists.apache.org/thread.html/08b64d3b084c865399f98f6c6f56235ce5329e843d97938e1a8045a5%40%3Cdev.airflow.apache.org%3E
>> > > >> > > > > > > >
>> > > >> > > > > > > > Based on the discussion with devlist, we want to delay
>> > > >> migrations
>> > > >> > > > to
>> > > >> > > > > the
>> > > >> > > > > > > > new assert statement.
>> > > >> > > > > > > >
>> > > >> > > > > > > > 4) Do we want to remove inheritance from
>> unittest.TestCase?
>> > > >> > > > > > > >
>> > > >> > > > > > > > 5) Do we want to use class-less tests?
>> > > >> > > > > > > >
>> > > >> > > > > > > > 6) Do we want to use pytest function instead of
>> current?
>> > > >> > > > > > > >
>> > > >> > > > > > > > For example:
>> > > >> > > > > > > >
>> > > >> > > > > > > > Unittest.assertRaises vs pytest.raises
>> > > >> > > > > > > >
>> > > >> > > > > > > > parametrize vs pytest.mark.parametrize
>> > > >> > > > > > > >
>> > > >> > > > > > > > unittest.skip[If], vs pytest.mark.skip[If]
>> > > >> > > > > > > >
>> > > >> > > > > > > > 7) Do we want to use monkeypatch fixture?
>> > > >> > > > > > > >
>> > > >> > > > > > > > https://docs.pytest.org/en/latest/monkeypatch.html
>> > > >> > > > > > > >
>> > > >> > > > > > > > 8) Do we want to use the pytest fixtures?
>> > > >> > > > > > > >
>> > > >> > > > > > > > Do we want to migrate all code to pytest fixtures?
>> > > >> > > > > > > >
>> > > >> > > > > > > > We are currently using a different style of fixtures.
>> Do we
>> > > >> want
>> > > >> > > to
>> > > >> > > > > give
>> > > >> > > > > > > > it up?
>> > > >> > > > > > > >
>> > > >> > > > > > > >
>> > > >> > > > > > >
>> > > >> > > > >
>> > > >> > > >
>> > > >> > >
>> > > >>
>> https://docs.python.org/3/library/unittest.html#class-and-module-fixtures
>> > > >> > > > > > > >
>> > > >> > > > > > > >
>> > > >> > > > > > > > It is worth asking to think about whether we do not
>> want to
>> > > >> > > change
>> > > >> > > > > this
>> > > >> > > > > > > > convention in the future e.g. after dropping support
>> for
>> > > >> 1.10.X.
>> > > >> > > We
>> > > >> > > > > can
>> > > >> > > > > > > > also allow two conventions on a temporary basis, and
>> then
>> > > >> migrate
>> > > >> > > > to
>> > > >> > > > > one
>> > > >> > > > > > > at
>> > > >> > > > > > > > a later time. For example, we want to migrate to the
>> assert
>> > > >> > > > statement
>> > > >> > > > > > > after
>> > > >> > > > > > > > dropping support for 1.10
>> > > >> > > > > > > >
>> > > >> > > > > > > >
>> > > >> > > > > > > > I hope I found the main differences between the
>> current
>> > > >> > > convention
>> > > >> > > > > and
>> > > >> > > > > > > the
>> > > >> > > > > > > > new convention. However, if you missed something,
>> please
>> > > >> continue
>> > > >> > > > to
>> > > >> > > > > > > number.
>> > > >> > > > > > > >
>> > > >> > > > > > > >
>> > > >> > > > > > > > Best regards,
>> > > >> > > > > > > >
>> > > >> > > > > > > > Kamil
>> > > >> > > > > > > >
>> > > >> > > > > > > >
>> > > >> > > > > > > >
>> > > >> > > > > > > >
>> > > >> > > > > > >
>> > > >> > > > >
>> > > >> > > >
>> > > >> > >
>> > > >> > >
>> > > >> > > --
>> > > >> > >
>> > > >> > > 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/> | Software Engineer
>> > > >> >
>> > > >> > M: +48 505 628 493 <+48505628493>
>> > > >> > E: tomasz.urbaszek@polidea.com <to...@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/>
>> >
>>
>>

Re: Test style convention after migration to pytest

Posted by Tomasz Urbaszek <tu...@apache.org>.

Hello all!

The PR for changing self.assert to plain assert is here:
https://github.com/apache/airflow/pull/12951

All tests seem to be passing (except on heisentest) so I'm going to merge
on Monday if there are no objections.

Cheers,
Tomek

On Thu, Oct 22, 2020 at 1:04 PM Ash Berlin-Taylor <as...@apache.org> wrote:

> 2. We already do don't we for backend and integrations etc?
> 3. I am strongly in favour of -- using assert  with pytest gives much
> nicer error messages than unittest's own assertions;
> 4. if we do 3, then I'm also in favour of doing 4 as we don't need it
> anymore, and we can then use more plugins/features of pytest (parameterized
> behaves better I think)
> 5. No view for or against
> 6. depends on what we do for 4
> 7. unittest.mock can be confusing to use, but I'm not sure pytests
> monkeypatch fixture is nicer. No opinoin on this one
> 8. I like them, but the automatic dependency injection can be confusing
> until you are familar with Pytest. An example of them for anyone unaware:
> https://github.com/apache/airflow/pull/11694 (look at the example test at
> the end of the PR description)
>
> I don't think we need to (yet) do any bulk changes to the test, but if we
> agree that using more of pytest is the way to go then we can write new
> tests using assert style, and once we've got a good chunk of the way we can
> bulk switch the rest.
>
> -ash
>
> On Oct 22 2020, at 11:45 am, Tomasz Urbaszek <tu...@apache.org> wrote:
>
> Hi all,
>
> I would like to resurrect this discussion as another one was started in
> https://github.com/apache/airflow/issues/11657#issuecomment-713834942
>
> Main questions were:
>
> > 1) Do we want to use additional plugins when there is no function in
> > the standard library e.g. flaky marker, forked marker?
> > 2) Do we want to use custom markers?
> > 3) Do we want to use new assert statement?
> > 4) Do we want to remove inheritance from unittest.TestCase?
> > 5) Do we want to use class-less tests?
> > 6) Do we want to use pytest function instead of current?
> > 7) Do we want to use monkeypatch fixture?
> > 8) Do we want to use the pytest fixtures?
>
> Probably any changes we decide on should be enforced after 2.0.
>
> Cheers,
> Tomek
>
> On 2020/01/18 01:08:29, Kamil Breguła <ka...@polidea.com> wrote:
> > Hello,
> >
> > Sorry for the late reply. Recently, I have been very busy with the
> > client project and work on the AIP-21.
> >
> > Let's start. I split this message into several sections for easier
> reading.
> >
> > **Establishment of a convention**
> > I will prepare a summary. It is available here:
> >
> >
> https://docs.google.com/spreadsheets/d/10qToeTFMxjShBX3xcK6IWr9Sp8Pw7qk35fQTtzpZYYs/edit?usp=sharing
> >
> > The mailing list does not allow rich content, so I copy the verdicts.
> >
> > 1) Do we want to use additional plugins when there is no function in
> > the standard library e.g. flaky marker, forked marker?YES
> > 2) Do we want to use custom markers? YES
> > 3) Do we want to use new assert statement? YES
> > 4) Do we want to remove inheritance from unittest.TestCase? YES
> > 5) Do we want to use class-less tests? NO
> > 6) Do we want to use pytest function instead of current? No verdict
> > 7) Do we want to use monkeypatch fixture? NO
> > 8) Do we want to use the pytest fixtures? YES
> >
> > We don't have a verdict in question 6, because we have no answer from
> > Tomek and Jarek. Can you answer this question? A detailed explanation
> > is available.
> >
> https://lists.apache.org/thread.html/7ae7ba0e72d3f0d12f8398a85980c5064b58574caa727c6a974fc628%40%3Cdev.airflow.apache.org%3E
> > Should we use the pytest functions or use the standard functions?
> >
> > **Using the new convention in the current code**
> > tl;dr; Two conventions will be accepted simultaneously.
> > Long story: As for the time of using the new convention, I'm the only
> > one who is worried about whether to allow it now. Other people did not
> > share my fears. So I think we should allow the use of the new
> > conventions now. We want to automate the introduction of the new
> > convention in the future, so I think that the application of the two
> > conventions should be allowed temporarily. However, I will describe
> > the new conventions, but we do not need to strictly require them until
> > the remaining code is migrated.
> >
> > **Full migration to the new convention**
> > tl;dr; We'll do it later.
> > Long story: Should we migrate the remaining code now? Tomek prefers
> > to wait for the end of work on AIP-21. Kaxil and I also prefer to wait
> > for AIP-21 or AIrflow 2.0. Jarek prefers to wait until we finish all
> > other works that improve the development environment e.g. pylint. So
> > we everybody prefers to wait. I think we can go back to the discussion
> > when the pylint is completed or we finish AIP-21. It depends on which
> > will be earlier.
> >
> > Best regards,
> > Kamil
> >
> > On Fri, Jan 17, 2020 at 3:07 PM Jarek Potiuk <Ja...@polidea.com>
> wrote:
> > >
> > > Should we resume this :)?
> > >
> > > On Mon, Jan 6, 2020 at 3:42 PM Jarek Potiuk <Ja...@polidea.com>
> > > wrote:
> > >
> > > > Good idea!
> > > >
> > > > On Mon, Jan 6, 2020 at 3:12 PM Kamil Breguła <
> kamil.bregula@polidea.com>
> > > > wrote:
> > > >
> > > >> Hello,
> > > >>
> > > >> Now is the holiday period. Some people have not started working yet.
> > > >> Others are busy with New Year activities. What do you think about
> > > >> Friday?
> > > >>
> > > >> Best regards,
> > > >> Kamil
> > > >>
> > > >> On Mon, Jan 6, 2020 at 2:48 PM Tomasz Urbaszek
> > > >> <to...@polidea.com> wrote:
> > > >> >
> > > >> > When should we assume that we've reached a consensus?
> > > >> >
> > > >> > T.
> > > >> >
> > > >> > On Thu, Jan 2, 2020 at 12:52 AM Jarek Potiuk <
> Jarek.Potiuk@polidea.com>
> > > >> > wrote:
> > > >> >
> > > >> > > > 1) Do we want to use additional plugins? *Yes*
> > > >> > >
> > > >> > > 2) Do we want to use custom markers? *Yes. They will help with
> > > >> optimising
> > > >> > > > our test execution.*
> > > >> > >
> > > >> > > 3) Do we want to use new assert statement? *Yes*
> > > >> > > > 4) Do we want to remove the inheritance from
> unittest.TestCase? *No
> > > >> > > > opinion about it. I am ok with both.*
> > > >> > > > 5) Do we want to use class-less tests? *No.*
> > > >> > > > 6) Do we want to use pytest function instead of current? *I
> don't
> > > >> > > > understand. Can you explain please?*
> > > >> > > > 7) Do we want to use monkeypatch fixture? *No. Mock is
> better.*
> > > >> > > > 8) Do we want to use the pytest fixtures? *Yes.*
> > > >> > > >
> > > >> > >
> > > >> > > Great that we have this discussion now. I also think we should
> just
> > > >> agree
> > > >> > > it now and not introduce it "globally".
> > > >> > > Once we do it, we should simply add it together new features we
> > > >> implement.
> > > >> > > We have still pylint to finish as a "non-functional global
> change"
> > > >> and we
> > > >> > > should not add new one. It's good to continually improve but one
> > > >> thing at a
> > > >> > > time.
> > > >> > >
> > > >> > > BTW Pylint goes well we are down to 243 non-pylint files from
> 991
> > > >> since
> > > >> > > May.
> > > >> > >
> > > >> > > J.
> > > >> > >
> > > >> > > On Thu, Jan 2, 2020 at 12:40 AM Kaxil Naik <kaxilnaik@gmail.com
> >
> > > >> wrote:
> > > >> > >
> > > >> > > > This is a tough one. Both arguments are reasonable.
> > > >> > > >
> > > >> > > > I agree at some point we should convert all to use assert.
> But at
> > > >> the
> > > >> > > same
> > > >> > > > time, we should also focus on adding *more user-facing
> features *and
> > > >> > > spend
> > > >> > > > less time on more refactor or similar changes.
> > > >> > > >
> > > >> > > > So based on that, this might be a low priority. We also need
> to
> > > >> still
> > > >> > > > complete AIP-21
> > > >> > > > <
> > > >> > > >
> > > >> > >
> > > >>
> https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-21%3A+Changes+in+import+paths
> > > >> > > > >
> > > >> > > > which is very critical for 2.0.
> > > >> > > >
> > > >> > > > 1) Do we want to use additional plugins?
> > > >> > > >
> > > >> > > >
> > > >> > > > Yes.
> > > >> > > >
> > > >> > > > 2) Do we want to use custom markers?
> > > >> > > >
> > > >> > > >
> > > >> > > > Not sure yet. Low Priority for me.
> > > >> > > >
> > > >> > > > 3) Do we want to use new assert statement?
> > > >> > > >
> > > >> > > >
> > > >> > > > I think new PRs can contain it, shouldn't be a problem as
> long as
> > > >> it is
> > > >> > > > documented to avoid confusion.
> > > >> > > >
> > > >> > > > 4) Do we want to remove the inheritance from
> unittest.TestCase?
> > > >> > > >
> > > >> > > >
> > > >> > > > Yes, this is, however, going to change how people write
> tests. So
> > > >> someone
> > > >> > > > has to own it as it can become painful with PRs getting merged
> > > >> > > > continuously.
> > > >> > > >
> > > >> > > > 5) Do we want to use class-less tests?
> > > >> > > >
> > > >> > > >
> > > >> > > > No.
> > > >> > > >
> > > >> > > > 6) Do we want to use pytest function instead of current?
> > > >> > > >
> > > >> > > >
> > > >> > > > Yes
> > > >> > > >
> > > >> > > > 7) Do we want to use monkeypatch fixture?
> > > >> > > >
> > > >> > > >
> > > >> > > > I also prefer unittest.mock but open to suggestions.
> > > >> > > >
> > > >>
> https://github.com/pytest-dev/pytest/issues/4576#issuecomment-449864333
> > > >> > > > has
> > > >> > > > some good comparison on it
> > > >> > > >
> > > >> > > > 8) Do we want to use the pytest fixtures?
> > > >> > > >
> > > >> > > >
> > > >> > > > Yes
> > > >> > > >
> > > >> > > > On Wed, Jan 1, 2020 at 8:07 PM Kamil Breguła <
> > > >> kamil.bregula@polidea.com>
> > > >> > > > wrote:
> > > >> > > >
> > > >> > > > > @unittest.skip("demonstrating skipping")
> > > >> > > > >
> > > >> > > > > On Wed, Jan 1, 2020 at 8:37 PM Tomasz Urbaszek <
> > > >> turbaszek@apache.org>
> > > >> > > > > wrote:
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > > > 6) Do we want to use pytest function instead of current?
> > > >> > > > > >
> > > >> > > > > > I do not understand this point, can you explain?
> > > >> > > > > >
> > > >> > > > >
> > > >> > > > > Pytest Introduces solutions that replace solutions that are
> now
> > > >> used
> > > >> > > > >
> > > >> > > > > For example:
> > > >> > > > > def test_foo(self):
> > > >> > > > > wtih self.assertRaises(AirflowException):
> > > >> > > > > bar()
> > > >> > > > >
> > > >> > > > > Can be replaced by following code:
> > > >> > > > >
> > > >> > > > > from pytest import raises
> > > >> > > > >
> > > >> > > > > wtih raises(AirflowException):
> > > >> > > > > bar()
> > > >> > > > >
> > > >> > > > > OR
> > > >> > > > >
> > > >> > > > > from parametrize import parametrize
> > > >> > > > >
> > > >> > > > > @parametrize.expand([
> > > >> > > > > (1, 1, ),
> > > >> > > > > (2, 2, ),
> > > >> > > > > ])
> > > >> > > > > def test_foo(self, param_a, param_b);
> > > >> > > > > self.assertEqual(param_a, param_b)
> > > >> > > > >
> > > >> > > > > can be replaced by
> > > >> > > > >
> > > >> > > > > @pytest.mark.parametrize("param_a,param_b", [(1, 1), (2,
> 2),])
> > > >> > > > > def test_eval(param_a, param_b):
> > > >> > > > > assert param_a == param_b
> > > >> > > > >
> > > >> > > > > OR
> > > >> > > > >
> > > >> > > > > @unittest.skip("demonstrating skipping")
> > > >> > > > > def test_foo(self)
> > > >> > > > >
> > > >> > > > > can be replaced by
> > > >> > > > >
> > > >> > > > > @pytest.mark.skip(reason="demonstrating skipping"))
> > > >> > > > > def test_foo(self)
> > > >> > > > >
> > > >> > > > > Which solution will be better for us?
> > > >> > > > >
> > > >> > > > > >
> > > >> > > > > > On Wed, Jan 1, 2020 at 8:14 PM Kamil Breguła <
> > > >> > > > kamil.bregula@polidea.com>
> > > >> > > > > > wrote:
> > > >> > > > > >
> > > >> > > > > > > > 1) Do we want to use additional plugins?
> > > >> > > > > > >
> > > >> > > > > > > Yes. We should use the full-power of plugins now.
> > > >> > > > > > >
> > > >> > > > > > > > 2) Do we want to use custom markers?
> > > >> > > > > > >
> > > >> > > > > > > Reply in a separate thread.
> > > >> > > > > > >
> > > >> > > > > > > >3) Do we want to use new assert statement?
> > > >> > > > > > >
> > > >> > > > > > > Reply in a separate thread
> > > >> > > > > > >
> > > >> > > > > > > > 4) Do we want to remove the inheritance from
> > > >> unittest.TestCase?
> > > >> > > > > > >
> > > >> > > > > > > Yes. After dropping support for Airflow 2.0, if
> possible. I
> > > >> would
> > > >> > > > > prefer to
> > > >> > > > > > > focus on working on new features for Airflow 2.0.
> > > >> > > > > > >
> > > >> > > > > > > > 5) Do we want to use class-less tests?
> > > >> > > > > > >
> > > >> > > > > > > No. Classes allow easy grouping of tests. Even if a
> file with
> > > >> one
> > > >> > > > > class now
> > > >> > > > > > > exists, a new one may appear in the future.
> > > >> > > > > > >
> > > >> > > > > > > > 6) Do we want to use pytest function instead of
> current?
> > > >> > > > > > >
> > > >> > > > > > > I feel good about the current functions. However, this
> is not
> > > >> a
> > > >> > > > serious
> > > >> > > > > > > relationship and I can create a new friendship.
> > > >> > > > > > >
> > > >> > > > > > > > 7) Do we want to use monkeypatch fixture?
> > > >> > > > > > >
> > > >> > > > > > > No. I prefer unittest.mock
> > > >> > > > > > >
> > > >> > > > > > > > 8) Do we want to use the pytest fixtures?
> > > >> > > > > > >
> > > >> > > > > > > No. I prefer classic fixtures, if possible. Their
> syntax is
> > > >> much
> > > >> > > > > simpler
> > > >> > > > > > > and easier to understand.
> > > >> > > > > > >
> > > >> > > > > > > On Wed, Jan 1, 2020 at 8:10 PM Kamil Breguła <
> > > >> > > > > kamil.bregula@polidea.com>
> > > >> > > > > > > wrote:
> > > >> > > > > > >
> > > >> > > > > > > > Hello,
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > > We have recently migrated to pytest. Code written
> > > >> according to
> > > >> > > the
> > > >> > > > > > > > standard library - unittest.TestCase is still
> compatible
> > > >> with
> > > >> > > > pytest.
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > > The AIP-21 document did not discuss the migration of
> > > >> current code
> > > >> > > > to
> > > >> > > > > > > > new features, but only discussed the change of runner
> and
> > > >> > > benefits
> > > >> > > > of
> > > >> > > > > > > > pytest over nosetets.
> > > >> > > > > > > >
> > > >> > > > > > > > Link:
> > > >> > > > > > > >
> > > >> > > > > > >
> > > >> > > > >
> > > >> > > >
> > > >> > >
> > > >>
> https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-27+Migrate+to+pytest
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > > As far as I appreciate the many advantages of using
> this
> > > >> tool,
> > > >> > > but
> > > >> > > > I
> > > >> > > > > am
> > > >> > > > > > > > not sure **whether, how or when we want to start
> using some
> > > >> > > > > features**. I
> > > >> > > > > > > > prefer to keep the current project conventions in many
> > > >> areas,
> > > >> > > but I
> > > >> > > > > still
> > > >> > > > > > > > love pytest. I think we should establish general
> > > >> conventions on
> > > >> > > > > writing
> > > >> > > > > > > > tests and recommended solutions to known problems. I
> prefer
> > > >> a
> > > >> > > > > pragmatic
> > > >> > > > > > > > approach, not just the use of features, because now
> we can
> > > >> use
> > > >> > > it.
> > > >> > > > > > > > Unfortunately, I do not see many benefits from new
> features.
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > > I would not like the code to have many ways of
> expressing
> > > >> the
> > > >> > > same
> > > >> > > > > > > > solution. For the following reasons:
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > > * it can introduce a lack of understanding among new
> > > >> contributors
> > > >> > > > > > > >
> > > >> > > > > > > > * facilitate the understanding and reading of code.
> > > >> > > > > > > >
> > > >> > > > > > > > * creates unnecessary discussion about the
> preferences of
> > > >> one way
> > > >> > > > > over
> > > >> > > > > > > the
> > > >> > > > > > > > other. Not related to changes.
> > > >> > > > > > > >
> > > >> > > > > > > > * forces an understanding of the complex syntax of
> some
> > > >> > > solutions.
> > > >> > > > > > > >
> > > >> > > > > > > > * encourages people to rewrite the code, which can
> generate
> > > >> > > > > unnecessary
> > > >> > > > > > > > work.
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > > To establish a full convention, I have prepared a few
> > > >> questions:
> > > >> > > > > > > >
> > > >> > > > > > > > 1) Do we want to use additional plugins when there is
> no
> > > >> function
> > > >> > > > in
> > > >> > > > > the
> > > >> > > > > > > > standard library e.g. flaky marker, forked marker?
> This
> > > >> is, in
> > > >> > > my
> > > >> > > > > > > opinion,
> > > >> > > > > > > > one of the big advantages of migrating to pytest.
> > > >> > > > > > > >
> > > >> > > > > > > > 2) Do we want to use custom markers?
> > > >> > > > > > > >
> > > >> > > > > > > > The discussion takes place in a separate thread:
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > >
> > > >> > > > >
> > > >> > > >
> > > >> > >
> > > >>
> https://lists.apache.org/thread.html/4538437c96f599766005ba7829d0bee1511debb4f53599e0d300a56f%40%3Cdev.airflow.apache.org%3E
> > > >> > > > > > > >
> > > >> > > > > > > > 3) Do we want to use new assert statement?
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > >
> > > >> > > > >
> > > >> > > >
> > > >> > >
> > > >>
> https://lists.apache.org/thread.html/08b64d3b084c865399f98f6c6f56235ce5329e843d97938e1a8045a5%40%3Cdev.airflow.apache.org%3E
> > > >> > > > > > > >
> > > >> > > > > > > > Based on the discussion with devlist, we want to delay
> > > >> migrations
> > > >> > > > to
> > > >> > > > > the
> > > >> > > > > > > > new assert statement.
> > > >> > > > > > > >
> > > >> > > > > > > > 4) Do we want to remove inheritance from
> unittest.TestCase?
> > > >> > > > > > > >
> > > >> > > > > > > > 5) Do we want to use class-less tests?
> > > >> > > > > > > >
> > > >> > > > > > > > 6) Do we want to use pytest function instead of
> current?
> > > >> > > > > > > >
> > > >> > > > > > > > For example:
> > > >> > > > > > > >
> > > >> > > > > > > > Unittest.assertRaises vs pytest.raises
> > > >> > > > > > > >
> > > >> > > > > > > > parametrize vs pytest.mark.parametrize
> > > >> > > > > > > >
> > > >> > > > > > > > unittest.skip[If], vs pytest.mark.skip[If]
> > > >> > > > > > > >
> > > >> > > > > > > > 7) Do we want to use monkeypatch fixture?
> > > >> > > > > > > >
> > > >> > > > > > > > https://docs.pytest.org/en/latest/monkeypatch.html
> > > >> > > > > > > >
> > > >> > > > > > > > 8) Do we want to use the pytest fixtures?
> > > >> > > > > > > >
> > > >> > > > > > > > Do we want to migrate all code to pytest fixtures?
> > > >> > > > > > > >
> > > >> > > > > > > > We are currently using a different style of fixtures.
> Do we
> > > >> want
> > > >> > > to
> > > >> > > > > give
> > > >> > > > > > > > it up?
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > >
> > > >> > > > >
> > > >> > > >
> > > >> > >
> > > >>
> https://docs.python.org/3/library/unittest.html#class-and-module-fixtures
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > > It is worth asking to think about whether we do not
> want to
> > > >> > > change
> > > >> > > > > this
> > > >> > > > > > > > convention in the future e.g. after dropping support
> for
> > > >> 1.10.X.
> > > >> > > We
> > > >> > > > > can
> > > >> > > > > > > > also allow two conventions on a temporary basis, and
> then
> > > >> migrate
> > > >> > > > to
> > > >> > > > > one
> > > >> > > > > > > at
> > > >> > > > > > > > a later time. For example, we want to migrate to the
> assert
> > > >> > > > statement
> > > >> > > > > > > after
> > > >> > > > > > > > dropping support for 1.10
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > > I hope I found the main differences between the
> current
> > > >> > > convention
> > > >> > > > > and
> > > >> > > > > > > the
> > > >> > > > > > > > new convention. However, if you missed something,
> please
> > > >> continue
> > > >> > > > to
> > > >> > > > > > > number.
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > > Best regards,
> > > >> > > > > > > >
> > > >> > > > > > > > Kamil
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > >
> > > >> > > > >
> > > >> > > >
> > > >> > >
> > > >> > >
> > > >> > > --
> > > >> > >
> > > >> > > 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/> | Software Engineer
> > > >> >
> > > >> > M: +48 505 628 493 <+48505628493>
> > > >> > E: tomasz.urbaszek@polidea.com <to...@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/>
> >
>
>

Re: Test style convention after migration to pytest

Posted by Ash Berlin-Taylor <as...@apache.org>.

2. We already do don't we for backend and integrations etc?
3. I am strongly in favour of -- using assert with pytest gives much nicer error messages than unittest's own assertions;
4. if we do 3, then I'm also in favour of doing 4 as we don't need it anymore, and we can then use more plugins/features of pytest (parameterized behaves better I think)
5. No view for or against
6. depends on what we do for 4
7. unittest.mock can be confusing to use, but I'm not sure pytests monkeypatch fixture is nicer. No opinoin on this one
8. I like them, but the automatic dependency injection can be confusing until you are familar with Pytest. An example of them for anyone unaware: https://github.com/apache/airflow/pull/11694 (look at the example test at the end of the PR description)

I don't think we need to (yet) do any bulk changes to the test, but if we agree that using more of pytest is the way to go then we can write new tests using assert style, and once we've got a good chunk of the way we can bulk switch the rest.
-ash
On Oct 22 2020, at 11:45 am, Tomasz Urbaszek <tu...@apache.org> wrote:
> Hi all,
>
> I would like to resurrect this discussion as another one was started in https://github.com/apache/airflow/issues/11657#issuecomment-713834942
> Main questions were:
> > 1) Do we want to use additional plugins when there is no function in
> > the standard library e.g. flaky marker, forked marker?
> > 2) Do we want to use custom markers?
> > 3) Do we want to use new assert statement?
> > 4) Do we want to remove inheritance from unittest.TestCase?
> > 5) Do we want to use class-less tests?
> > 6) Do we want to use pytest function instead of current?
> > 7) Do we want to use monkeypatch fixture?
> > 8) Do we want to use the pytest fixtures?
>
> Probably any changes we decide on should be enforced after 2.0.
> Cheers,
> Tomek
>
> On 2020/01/18 01:08:29, Kamil Breguła <ka...@polidea.com> wrote:
> > Hello,
> >
> > Sorry for the late reply. Recently, I have been very busy with the
> > client project and work on the AIP-21.
> >
> > Let's start. I split this message into several sections for easier reading.
> >
> > **Establishment of a convention**
> > I will prepare a summary. It is available here:
> >
> > https://docs.google.com/spreadsheets/d/10qToeTFMxjShBX3xcK6IWr9Sp8Pw7qk35fQTtzpZYYs/edit?usp=sharing
> >
> > The mailing list does not allow rich content, so I copy the verdicts.
> >
> > 1) Do we want to use additional plugins when there is no function in
> > the standard library e.g. flaky marker, forked marker?YES
> > 2) Do we want to use custom markers? YES
> > 3) Do we want to use new assert statement? YES
> > 4) Do we want to remove inheritance from unittest.TestCase? YES
> > 5) Do we want to use class-less tests? NO
> > 6) Do we want to use pytest function instead of current? No verdict
> > 7) Do we want to use monkeypatch fixture? NO
> > 8) Do we want to use the pytest fixtures? YES
> >
> > We don't have a verdict in question 6, because we have no answer from
> > Tomek and Jarek. Can you answer this question? A detailed explanation
> > is available.
> > https://lists.apache.org/thread.html/7ae7ba0e72d3f0d12f8398a85980c5064b58574caa727c6a974fc628%40%3Cdev.airflow.apache.org%3E
> > Should we use the pytest functions or use the standard functions?
> >
> > **Using the new convention in the current code**
> > tl;dr; Two conventions will be accepted simultaneously.
> > Long story: As for the time of using the new convention, I'm the only
> > one who is worried about whether to allow it now. Other people did not
> > share my fears. So I think we should allow the use of the new
> > conventions now. We want to automate the introduction of the new
> > convention in the future, so I think that the application of the two
> > conventions should be allowed temporarily. However, I will describe
> > the new conventions, but we do not need to strictly require them until
> > the remaining code is migrated.
> >
> > **Full migration to the new convention**
> > tl;dr; We'll do it later.
> > Long story: Should we migrate the remaining code now? Tomek prefers
> > to wait for the end of work on AIP-21. Kaxil and I also prefer to wait
> > for AIP-21 or AIrflow 2.0. Jarek prefers to wait until we finish all
> > other works that improve the development environment e.g. pylint. So
> > we everybody prefers to wait. I think we can go back to the discussion
> > when the pylint is completed or we finish AIP-21. It depends on which
> > will be earlier.
> >
> > Best regards,
> > Kamil
> >
> > On Fri, Jan 17, 2020 at 3:07 PM Jarek Potiuk <Ja...@polidea.com> wrote:
> > >
> > > Should we resume this :)?
> > >
> > > On Mon, Jan 6, 2020 at 3:42 PM Jarek Potiuk <Ja...@polidea.com>
> > > wrote:
> > >
> > > > Good idea!
> > > >
> > > > On Mon, Jan 6, 2020 at 3:12 PM Kamil Breguła <ka...@polidea.com>
> > > > wrote:
> > > >
> > > >> Hello,
> > > >>
> > > >> Now is the holiday period. Some people have not started working yet.
> > > >> Others are busy with New Year activities. What do you think about
> > > >> Friday?
> > > >>
> > > >> Best regards,
> > > >> Kamil
> > > >>
> > > >> On Mon, Jan 6, 2020 at 2:48 PM Tomasz Urbaszek
> > > >> <to...@polidea.com> wrote:
> > > >> >
> > > >> > When should we assume that we've reached a consensus?
> > > >> >
> > > >> > T.
> > > >> >
> > > >> > On Thu, Jan 2, 2020 at 12:52 AM Jarek Potiuk <Ja...@polidea.com>
> > > >> > wrote:
> > > >> >
> > > >> > > > 1) Do we want to use additional plugins? *Yes*
> > > >> > >
> > > >> > > 2) Do we want to use custom markers? *Yes. They will help with
> > > >> optimising
> > > >> > > > our test execution.*
> > > >> > >
> > > >> > > 3) Do we want to use new assert statement? *Yes*
> > > >> > > > 4) Do we want to remove the inheritance from unittest.TestCase? *No
> > > >> > > > opinion about it. I am ok with both.*
> > > >> > > > 5) Do we want to use class-less tests? *No.*
> > > >> > > > 6) Do we want to use pytest function instead of current? *I don't
> > > >> > > > understand. Can you explain please?*
> > > >> > > > 7) Do we want to use monkeypatch fixture? *No. Mock is better.*
> > > >> > > > 8) Do we want to use the pytest fixtures? *Yes.*
> > > >> > > >
> > > >> > >
> > > >> > > Great that we have this discussion now. I also think we should just
> > > >> agree
> > > >> > > it now and not introduce it "globally".
> > > >> > > Once we do it, we should simply add it together new features we
> > > >> implement.
> > > >> > > We have still pylint to finish as a "non-functional global change"
> > > >> and we
> > > >> > > should not add new one. It's good to continually improve but one
> > > >> thing at a
> > > >> > > time.
> > > >> > >
> > > >> > > BTW Pylint goes well we are down to 243 non-pylint files from 991
> > > >> since
> > > >> > > May.
> > > >> > >
> > > >> > > J.
> > > >> > >
> > > >> > > On Thu, Jan 2, 2020 at 12:40 AM Kaxil Naik <ka...@gmail.com>
> > > >> wrote:
> > > >> > >
> > > >> > > > This is a tough one. Both arguments are reasonable.
> > > >> > > >
> > > >> > > > I agree at some point we should convert all to use assert. But at
> > > >> the
> > > >> > > same
> > > >> > > > time, we should also focus on adding *more user-facing features *and
> > > >> > > spend
> > > >> > > > less time on more refactor or similar changes.
> > > >> > > >
> > > >> > > > So based on that, this might be a low priority. We also need to
> > > >> still
> > > >> > > > complete AIP-21
> > > >> > > > <
> > > >> > > >
> > > >> > >
> > > >> https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-21%3A+Changes+in+import+paths
> > > >> > > > >
> > > >> > > > which is very critical for 2.0.
> > > >> > > >
> > > >> > > > 1) Do we want to use additional plugins?
> > > >> > > >
> > > >> > > >
> > > >> > > > Yes.
> > > >> > > >
> > > >> > > > 2) Do we want to use custom markers?
> > > >> > > >
> > > >> > > >
> > > >> > > > Not sure yet. Low Priority for me.
> > > >> > > >
> > > >> > > > 3) Do we want to use new assert statement?
> > > >> > > >
> > > >> > > >
> > > >> > > > I think new PRs can contain it, shouldn't be a problem as long as
> > > >> it is
> > > >> > > > documented to avoid confusion.
> > > >> > > >
> > > >> > > > 4) Do we want to remove the inheritance from unittest.TestCase?
> > > >> > > >
> > > >> > > >
> > > >> > > > Yes, this is, however, going to change how people write tests. So
> > > >> someone
> > > >> > > > has to own it as it can become painful with PRs getting merged
> > > >> > > > continuously.
> > > >> > > >
> > > >> > > > 5) Do we want to use class-less tests?
> > > >> > > >
> > > >> > > >
> > > >> > > > No.
> > > >> > > >
> > > >> > > > 6) Do we want to use pytest function instead of current?
> > > >> > > >
> > > >> > > >
> > > >> > > > Yes
> > > >> > > >
> > > >> > > > 7) Do we want to use monkeypatch fixture?
> > > >> > > >
> > > >> > > >
> > > >> > > > I also prefer unittest.mock but open to suggestions.
> > > >> > > >
> > > >> https://github.com/pytest-dev/pytest/issues/4576#issuecomment-449864333
> > > >> > > > has
> > > >> > > > some good comparison on it
> > > >> > > >
> > > >> > > > 8) Do we want to use the pytest fixtures?
> > > >> > > >
> > > >> > > >
> > > >> > > > Yes
> > > >> > > >
> > > >> > > > On Wed, Jan 1, 2020 at 8:07 PM Kamil Breguła <
> > > >> kamil.bregula@polidea.com>
> > > >> > > > wrote:
> > > >> > > >
> > > >> > > > > @unittest.skip("demonstrating skipping")
> > > >> > > > >
> > > >> > > > > On Wed, Jan 1, 2020 at 8:37 PM Tomasz Urbaszek <
> > > >> turbaszek@apache.org>
> > > >> > > > > wrote:
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > > > 6) Do we want to use pytest function instead of current?
> > > >> > > > > >
> > > >> > > > > > I do not understand this point, can you explain?
> > > >> > > > > >
> > > >> > > > >
> > > >> > > > > Pytest Introduces solutions that replace solutions that are now
> > > >> used
> > > >> > > > >
> > > >> > > > > For example:
> > > >> > > > > def test_foo(self):
> > > >> > > > > wtih self.assertRaises(AirflowException):
> > > >> > > > > bar()
> > > >> > > > >
> > > >> > > > > Can be replaced by following code:
> > > >> > > > >
> > > >> > > > > from pytest import raises
> > > >> > > > >
> > > >> > > > > wtih raises(AirflowException):
> > > >> > > > > bar()
> > > >> > > > >
> > > >> > > > > OR
> > > >> > > > >
> > > >> > > > > from parametrize import parametrize
> > > >> > > > >
> > > >> > > > > @parametrize.expand([
> > > >> > > > > (1, 1, ),
> > > >> > > > > (2, 2, ),
> > > >> > > > > ])
> > > >> > > > > def test_foo(self, param_a, param_b);
> > > >> > > > > self.assertEqual(param_a, param_b)
> > > >> > > > >
> > > >> > > > > can be replaced by
> > > >> > > > >
> > > >> > > > > @pytest.mark.parametrize("param_a,param_b", [(1, 1), (2, 2),])
> > > >> > > > > def test_eval(param_a, param_b):
> > > >> > > > > assert param_a == param_b
> > > >> > > > >
> > > >> > > > > OR
> > > >> > > > >
> > > >> > > > > @unittest.skip("demonstrating skipping")
> > > >> > > > > def test_foo(self)
> > > >> > > > >
> > > >> > > > > can be replaced by
> > > >> > > > >
> > > >> > > > > @pytest.mark.skip(reason="demonstrating skipping"))
> > > >> > > > > def test_foo(self)
> > > >> > > > >
> > > >> > > > > Which solution will be better for us?
> > > >> > > > >
> > > >> > > > > >
> > > >> > > > > > On Wed, Jan 1, 2020 at 8:14 PM Kamil Breguła <
> > > >> > > > kamil.bregula@polidea.com>
> > > >> > > > > > wrote:
> > > >> > > > > >
> > > >> > > > > > > > 1) Do we want to use additional plugins?
> > > >> > > > > > >
> > > >> > > > > > > Yes. We should use the full-power of plugins now.
> > > >> > > > > > >
> > > >> > > > > > > > 2) Do we want to use custom markers?
> > > >> > > > > > >
> > > >> > > > > > > Reply in a separate thread.
> > > >> > > > > > >
> > > >> > > > > > > >3) Do we want to use new assert statement?
> > > >> > > > > > >
> > > >> > > > > > > Reply in a separate thread
> > > >> > > > > > >
> > > >> > > > > > > > 4) Do we want to remove the inheritance from
> > > >> unittest.TestCase?
> > > >> > > > > > >
> > > >> > > > > > > Yes. After dropping support for Airflow 2.0, if possible. I
> > > >> would
> > > >> > > > > prefer to
> > > >> > > > > > > focus on working on new features for Airflow 2.0.
> > > >> > > > > > >
> > > >> > > > > > > > 5) Do we want to use class-less tests?
> > > >> > > > > > >
> > > >> > > > > > > No. Classes allow easy grouping of tests. Even if a file with
> > > >> one
> > > >> > > > > class now
> > > >> > > > > > > exists, a new one may appear in the future.
> > > >> > > > > > >
> > > >> > > > > > > > 6) Do we want to use pytest function instead of current?
> > > >> > > > > > >
> > > >> > > > > > > I feel good about the current functions. However, this is not
> > > >> a
> > > >> > > > serious
> > > >> > > > > > > relationship and I can create a new friendship.
> > > >> > > > > > >
> > > >> > > > > > > > 7) Do we want to use monkeypatch fixture?
> > > >> > > > > > >
> > > >> > > > > > > No. I prefer unittest.mock
> > > >> > > > > > >
> > > >> > > > > > > > 8) Do we want to use the pytest fixtures?
> > > >> > > > > > >
> > > >> > > > > > > No. I prefer classic fixtures, if possible. Their syntax is
> > > >> much
> > > >> > > > > simpler
> > > >> > > > > > > and easier to understand.
> > > >> > > > > > >
> > > >> > > > > > > On Wed, Jan 1, 2020 at 8:10 PM Kamil Breguła <
> > > >> > > > > kamil.bregula@polidea.com>
> > > >> > > > > > > wrote:
> > > >> > > > > > >
> > > >> > > > > > > > Hello,
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > > We have recently migrated to pytest. Code written
> > > >> according to
> > > >> > > the
> > > >> > > > > > > > standard library - unittest.TestCase is still compatible
> > > >> with
> > > >> > > > pytest.
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > > The AIP-21 document did not discuss the migration of
> > > >> current code
> > > >> > > > to
> > > >> > > > > > > > new features, but only discussed the change of runner and
> > > >> > > benefits
> > > >> > > > of
> > > >> > > > > > > > pytest over nosetets.
> > > >> > > > > > > >
> > > >> > > > > > > > Link:
> > > >> > > > > > > >
> > > >> > > > > > >
> > > >> > > > >
> > > >> > > >
> > > >> > >
> > > >> https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-27+Migrate+to+pytest
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > > As far as I appreciate the many advantages of using this
> > > >> tool,
> > > >> > > but
> > > >> > > > I
> > > >> > > > > am
> > > >> > > > > > > > not sure **whether, how or when we want to start using some
> > > >> > > > > features**. I
> > > >> > > > > > > > prefer to keep the current project conventions in many
> > > >> areas,
> > > >> > > but I
> > > >> > > > > still
> > > >> > > > > > > > love pytest. I think we should establish general
> > > >> conventions on
> > > >> > > > > writing
> > > >> > > > > > > > tests and recommended solutions to known problems. I prefer
> > > >> a
> > > >> > > > > pragmatic
> > > >> > > > > > > > approach, not just the use of features, because now we can
> > > >> use
> > > >> > > it.
> > > >> > > > > > > > Unfortunately, I do not see many benefits from new features.
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > > I would not like the code to have many ways of expressing
> > > >> the
> > > >> > > same
> > > >> > > > > > > > solution. For the following reasons:
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > > * it can introduce a lack of understanding among new
> > > >> contributors
> > > >> > > > > > > >
> > > >> > > > > > > > * facilitate the understanding and reading of code.
> > > >> > > > > > > >
> > > >> > > > > > > > * creates unnecessary discussion about the preferences of
> > > >> one way
> > > >> > > > > over
> > > >> > > > > > > the
> > > >> > > > > > > > other. Not related to changes.
> > > >> > > > > > > >
> > > >> > > > > > > > * forces an understanding of the complex syntax of some
> > > >> > > solutions.
> > > >> > > > > > > >
> > > >> > > > > > > > * encourages people to rewrite the code, which can generate
> > > >> > > > > unnecessary
> > > >> > > > > > > > work.
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > > To establish a full convention, I have prepared a few
> > > >> questions:
> > > >> > > > > > > >
> > > >> > > > > > > > 1) Do we want to use additional plugins when there is no
> > > >> function
> > > >> > > > in
> > > >> > > > > the
> > > >> > > > > > > > standard library e.g. flaky marker, forked marker? This
> > > >> is, in
> > > >> > > my
> > > >> > > > > > > opinion,
> > > >> > > > > > > > one of the big advantages of migrating to pytest.
> > > >> > > > > > > >
> > > >> > > > > > > > 2) Do we want to use custom markers?
> > > >> > > > > > > >
> > > >> > > > > > > > The discussion takes place in a separate thread:
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > >
> > > >> > > > >
> > > >> > > >
> > > >> > >
> > > >> https://lists.apache.org/thread.html/4538437c96f599766005ba7829d0bee1511debb4f53599e0d300a56f%40%3Cdev.airflow.apache.org%3E
> > > >> > > > > > > >
> > > >> > > > > > > > 3) Do we want to use new assert statement?
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > >
> > > >> > > > >
> > > >> > > >
> > > >> > >
> > > >> https://lists.apache.org/thread.html/08b64d3b084c865399f98f6c6f56235ce5329e843d97938e1a8045a5%40%3Cdev.airflow.apache.org%3E
> > > >> > > > > > > >
> > > >> > > > > > > > Based on the discussion with devlist, we want to delay
> > > >> migrations
> > > >> > > > to
> > > >> > > > > the
> > > >> > > > > > > > new assert statement.
> > > >> > > > > > > >
> > > >> > > > > > > > 4) Do we want to remove inheritance from unittest.TestCase?
> > > >> > > > > > > >
> > > >> > > > > > > > 5) Do we want to use class-less tests?
> > > >> > > > > > > >
> > > >> > > > > > > > 6) Do we want to use pytest function instead of current?
> > > >> > > > > > > >
> > > >> > > > > > > > For example:
> > > >> > > > > > > >
> > > >> > > > > > > > Unittest.assertRaises vs pytest.raises
> > > >> > > > > > > >
> > > >> > > > > > > > parametrize vs pytest.mark.parametrize
> > > >> > > > > > > >
> > > >> > > > > > > > unittest.skip[If], vs pytest.mark.skip[If]
> > > >> > > > > > > >
> > > >> > > > > > > > 7) Do we want to use monkeypatch fixture?
> > > >> > > > > > > >
> > > >> > > > > > > > https://docs.pytest.org/en/latest/monkeypatch.html
> > > >> > > > > > > >
> > > >> > > > > > > > 8) Do we want to use the pytest fixtures?
> > > >> > > > > > > >
> > > >> > > > > > > > Do we want to migrate all code to pytest fixtures?
> > > >> > > > > > > >
> > > >> > > > > > > > We are currently using a different style of fixtures. Do we
> > > >> want
> > > >> > > to
> > > >> > > > > give
> > > >> > > > > > > > it up?
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > >
> > > >> > > > >
> > > >> > > >
> > > >> > >
> > > >> https://docs.python.org/3/library/unittest.html#class-and-module-fixtures
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > > It is worth asking to think about whether we do not want to
> > > >> > > change
> > > >> > > > > this
> > > >> > > > > > > > convention in the future e.g. after dropping support for
> > > >> 1.10.X.
> > > >> > > We
> > > >> > > > > can
> > > >> > > > > > > > also allow two conventions on a temporary basis, and then
> > > >> migrate
> > > >> > > > to
> > > >> > > > > one
> > > >> > > > > > > at
> > > >> > > > > > > > a later time. For example, we want to migrate to the assert
> > > >> > > > statement
> > > >> > > > > > > after
> > > >> > > > > > > > dropping support for 1.10
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > > I hope I found the main differences between the current
> > > >> > > convention
> > > >> > > > > and
> > > >> > > > > > > the
> > > >> > > > > > > > new convention. However, if you missed something, please
> > > >> continue
> > > >> > > > to
> > > >> > > > > > > number.
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > > Best regards,
> > > >> > > > > > > >
> > > >> > > > > > > > Kamil
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > >
> > > >> > > > >
> > > >> > > >
> > > >> > >
> > > >> > >
> > > >> > > --
> > > >> > >
> > > >> > > 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/> | Software Engineer
> > > >> >
> > > >> > M: +48 505 628 493 <+48505628493>
> > > >> > E: tomasz.urbaszek@polidea.com <to...@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/>
> >
>