Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In unit tests, Application comparisons can report false positive #1866

Closed
Anvil opened this issue May 3, 2017 · 3 comments
Closed

In unit tests, Application comparisons can report false positive #1866

Anvil opened this issue May 3, 2017 · 3 comments
Labels

Comments

@Anvil
Copy link
Contributor

Anvil commented May 3, 2017

Comparison between Application is performed at the MutableMapping level. MutableMapping says that, like dict objects, if all keys and matching values are the same 2 instances, then they are equals. This means that web.Application() == web.Application() will return True.

See:

>>> a = aiohttp.web.Application()
>>> b = aiohttp.web.Application()
>>> a == b
True
>>> a["foo"] = "bar"
>>> a == b
False
>>> b["foo"] = "bar"
>>> a == b
True

I think those few unit tests are assuming a different behaviour:

  • test_subapp_middlewares
  • test_subapp_on_response_prepare
  • test_subapp_on_startup
  • test_subapp_on_shutdown
  • test_subapp_on_cleanup

A change has been submitted for test_subapp_middlewares in #1854 to fix that. While the solution may or may not be accepted as is, all tests should be fixed.

Also, maybe an additional test_application_equal should be implemented, to be ensure expected behavior. Unless web.Application.__eq__ special method gets implemented to change current behaviour, it should look like something like that:

def test_application_equal():
    app1 = web.Application()
    app2 = web.Application()
    assert app1 == app2
    app1["foo"] = "bar"
    assert app1 != app2
@asvetlov
Copy link
Member

asvetlov commented May 3, 2017

You've raised an interesting problem.

  1. We could leave apps comparison as is but this way will constantly produce many subtle bugs.
  2. On other hand we could forbid applications comparison in the way that apps are equal if and only if they are the same (app1 is app2). Ordering should be fixed as well.

I'm inclining to option 2 (all potentially broken tests should be fixed as well).
I believe the change will not affect our users: most likely the only case for app comparison is test writing.

@Anvil
Copy link
Contributor Author

Anvil commented May 10, 2017

For the record (and as far as my opinion matters), I would agree with option 2.

@lock
Copy link

lock bot commented Oct 28, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants