Skip to content

Use plain asserts in tests.#12951

Merged
potiuk merged 1 commit into
apache:masterfrom
jmcarp:jmcarp/pytest-asserts
Jan 17, 2021
Merged

Use plain asserts in tests.#12951
potiuk merged 1 commit into
apache:masterfrom
jmcarp:jmcarp/pytest-asserts

Conversation

@jmcarp

@jmcarp jmcarp commented Dec 9, 2020

Copy link
Copy Markdown
Contributor

We have talked about switching from unittest-style assertions (like self.assertEqual(foo, bar)) to plain pytest-style assertions (like assert foo == bar) on slack and the mailing list, but it doesn't seem like we've decided so far. I wanted to send a sample PR to show how this might work. I generated these changes with a one-line command:

unittest2pytest -f self_assert tests/core -w

If we try to refactor many tests at once, we'll likely run into conflicts, but because we can auto-refactor the code easily, fixing conflicts should be straightforward too.

Note that I didn't use the remove_class fixer or move test setup into fixtures--this just concerns assertion style.

Does this seem like an improvement? If so, we can refactor tests module by module, or run unittest2pytest on all the tests at once.

cc @ashb @potiuk

@github-actions

github-actions Bot commented Dec 9, 2020

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions

github-actions Bot commented Dec 9, 2020

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. Building image for the PR has been cancelled

@turbaszek

Copy link
Copy Markdown
Member

Thanks @jmcarp for the PR!

doesn't seem like we've decided so far

We've reached consensus that we prefer assert x == y but it was postponed until we release and stabilise Airflow 2.0.

@jmcarp jmcarp force-pushed the jmcarp/pytest-asserts branch 3 times, most recently from 4acf381 to 65f45d3 Compare December 25, 2020 23:43
@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@jmcarp jmcarp force-pushed the jmcarp/pytest-asserts branch 5 times, most recently from 9f6acb8 to 5283851 Compare December 26, 2020 18:36
@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@jmcarp jmcarp force-pushed the jmcarp/pytest-asserts branch 17 times, most recently from 9831e7c to 8a9689a Compare December 28, 2020 15:24
@potiuk

potiuk commented Jan 10, 2021

Copy link
Copy Markdown
Member

The failed test is https://github.com/apache/airflow/pull/12951/checks?check_run_id=1675678142#step:6:890 - looking at it, I think it either should be moved to quarantine or fixed.

@jmcarp jmcarp force-pushed the jmcarp/pytest-asserts branch from 3e0c0f2 to faeb5ea Compare January 10, 2021 16:15
@jmcarp

jmcarp commented Jan 10, 2021

Copy link
Copy Markdown
Contributor Author

I'm marking that test as quarantined. It passes locally, and behavior seems inconsistent from CI.

@jmcarp jmcarp force-pushed the jmcarp/pytest-asserts branch from faeb5ea to 70425ad Compare January 10, 2021 23:04
@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. Building image for the PR has been cancelled

@jmcarp jmcarp force-pushed the jmcarp/pytest-asserts branch 2 times, most recently from 301b293 to 093ec51 Compare January 11, 2021 16:07
@jmcarp

jmcarp commented Jan 11, 2021

Copy link
Copy Markdown
Contributor Author

Rebased. The last failing tests I saw seemed random and unrelated to these changes.

@jmcarp jmcarp force-pushed the jmcarp/pytest-asserts branch from 093ec51 to 0bc7b2f Compare January 13, 2021 05:49
@turbaszek

Copy link
Copy Markdown
Member

There's an error which seems to be present in multiple builds, so I think we should take a look at that:

  tests/core/test_configuration.py:518: 
915
  _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
916
  
917
  self = <tests.test_utils.reset_warning_registry.reset_warning_registry object at 0x7f0b78239f50>
918
  
919
      def __enter__(self):
920
          # archive and clear the __warningregistry__ key for all modules
921
          # that match the 'reset' pattern.
922
          pattern = self._pattern
923
          backup = self._backup = {}
924
          for name, mod in list(sys.modules.items()):
925
              if pattern.match(name):
926
                  reg = getattr(mod, "__warningregistry__", None)
927
                  if reg:
928
  >                   backup[name] = reg.copy()
929
  E                   AttributeError: 'str' object has no attribute 'copy'

@jmcarp jmcarp force-pushed the jmcarp/pytest-asserts branch 5 times, most recently from 5642146 to b7df0c9 Compare January 14, 2021 02:02
@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

@jmcarp jmcarp force-pushed the jmcarp/pytest-asserts branch 2 times, most recently from 5b77ef0 to e6cc6dd Compare January 14, 2021 05:08
@potiuk

potiuk commented Jan 14, 2021

Copy link
Copy Markdown
Member

Hey @jmcarp -> I had to cancel this run because of this random GitHub error https://github.com/apache/airflow/runs/1699834461?check_suite_focus=true#step:10:310

Can you please --amend/push again?

BTW. We have a meeting today with INFRA and GitHub developer advocate https://cwiki.apache.org/confluence/display/INFRA/ASF+Builds+Agenda+2021-01-14, as this is releted to erratic behaviour of Github Registry

@jmcarp jmcarp force-pushed the jmcarp/pytest-asserts branch from e6cc6dd to 69f6cfa Compare January 14, 2021 17:46
@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@jmcarp

jmcarp commented Jan 17, 2021

Copy link
Copy Markdown
Contributor Author

Ok, I think tests are in good shape and the failures are random (quarantined tests) or a formatting regression in setup.cfg that's already in the master branch.

@potiuk

potiuk commented Jan 17, 2021

Copy link
Copy Markdown
Member

Yeah. There Was an extra space (!) added in setup.cfg but it has been fixed since :)

@potiuk

potiuk commented Jan 17, 2021

Copy link
Copy Markdown
Member

Thanks for your patience @jmcarp! :) The CI problems did not make it easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

full tests needed We need to run full set of tests for this PR to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants