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 <a...@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 <turbas...@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 <kamil.breg...@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 <jarek.pot...@polidea.com> > wrote: > > > > > > Should we resume this :)? > > > > > > On Mon, Jan 6, 2020 at 3:42 PM Jarek Potiuk <jarek.pot...@polidea.com> > > > wrote: > > > > > > > Good idea! > > > > > > > > On Mon, Jan 6, 2020 at 3:12 PM Kamil Breguła < > kamil.breg...@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 > > > >> <tomasz.urbas...@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.pot...@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 <kaxiln...@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.breg...@polidea.com> > > > >> > > > wrote: > > > >> > > > > > > >> > > > > @unittest.skip("demonstrating skipping") > > > >> > > > > > > > >> > > > > On Wed, Jan 1, 2020 at 8:37 PM Tomasz Urbaszek < > > > >> turbas...@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.breg...@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.breg...@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.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/> > > > > > > > > > > > > > > -- > > > > > > Jarek Potiuk > > > Polidea <https://www.polidea.com/> | Principal Software Engineer > > > > > > M: +48 660 796 129 <+48660796129> > > > [image: Polidea] <https://www.polidea.com/> > > > >
