AssertionError in test_invalid_isbn
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Dalba
	Jan 19 2018, 2:50 PM

Description

Running on Windows, make sure you don't have python-stdnum installed.

$ python -m unittest tests.isbn_tests.TestCosmeticChangesISBN.test_invalid_isbn
tests: max_retries reduced from 5 to 1
WARNING: ...\Python36\lib\importlib\_bootstrap.py:219: ImportWarning: can't resolve package from
__spec__ or __package__, falling back on __name__ and __path__
  return f(*args, **kwds)

WARNING: ...\Python36\lib\site-packages\bs4\builder\_lxml.py:250: DeprecationWarning: inspect.get
argspec() is deprecated, use inspect.signature() or inspect.getfullargspec()
  self.parser.feed(markup)

WARNING: ...\pywikibot-core\pywikibot\cosmetic_changes.py:154: ImportWarning: package stdnum.isbn not foun
d; using scripts.isbn
  ImportWarning)

F
======================================================================
FAIL: test_invalid_isbn (tests.isbn_tests.TestCosmeticChangesISBN)
Test that it'll fail when the ISBN is invalid.
----------------------------------------------------------------------
scripts.isbn.InvalidIsbnException: Invalid ISBN found

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "...\pywikibot-core\tests\isbn_tests.py", line 79, in test_invalid_isbn
    cc.fix_ISBN, 'ISBN 0975229LOL')
AssertionError: "The ISBN ([A-Z0-9]*) is not (10|13) digits long|The number has an invalid length" does not match "Invalid ISBN found"


----------------------------------------------------------------------
Ran 1 test in 0.003s

FAILED (failures=1)

Details

	Subject	Repo	Branch	Lines +/-
	Revert "Replace assertRaises with assertRaisesRegex in isbn_tests.py"	pywikibot/core	master	+15 -53

Customize query in gerrit

Related Objects

Mentioned In: T154281: [recurring] Replace assertRaises with assertRaisesRegex
Mentioned Here: rPWBC9a13055ce9c1: Replace assertRaises with assertRaisesRegex in isbn_tests.py

Event Timeline

Dalba created this task.Jan 19 2018, 2:50 PM

Restricted Application added subscribers: pywikibot-bugs-list, Aklapper. · View Herald TranscriptJan 19 2018, 2:50 PM

The issue has appeared after 9a13055ce9c130d832ebd273681e7bf4d5dd71a7.

There is also another change request at https://gerrit.wikimedia.org/r/#/c/399348/. I had tested it on travis and it too seems to pass the travis tests. I'll probably will suggest to revert the one above.

Change 405292 had a related patch set uploaded (by Dalba; owner: Dalba):
[pywikibot/core@master] Revert "Replace assertRaises with assertRaisesRegex in isbn_tests.py"

https://gerrit.wikimedia.org/r/405292

gerritbot added a project: Patch-For-Review.Jan 19 2018, 3:08 PM

Change 405292 merged by jenkins-bot:
[pywikibot/core@master] Revert "Replace assertRaises with assertRaisesRegex in isbn_tests.py"

https://gerrit.wikimedia.org/r/405292

Dalba closed this task as Resolved.Jan 19 2018, 7:03 PM

Dalba claimed this task.

Running on Windows, make sure you don't have python-stdnum installed.

But isn't it a requirement to run Pywikibot as I can see in the requirements.txt?
https://github.com/wikimedia/pywikibot/blob/master/requirements.txt#L54

In my opinion this shouldn't be marked as a bug to be fixed, but rather a bug to be solved by the user by installing the required module.

this shouldn't be marked as a bug to be fixed, but rather a bug to be solved by the user by installing the required module.

In T185317#3913465, @divadsn wrote:

But isn't it a requirement to run Pywikibot as I can see in the requirements.txt?

Not really, the only real requirement to run pywikibot is requests. Most of the other requirements are extras for specific scripts. See install_requires and tests_require in setup.py. python-stdnum is not in tests_require.

My understanding is that if a test module depends on another module that is not defined in test_requires then the require_modules decorator (defined in aspects.py) should be used for that test.

Afaik it's usually supposed to install all needed requirements from the requirements.txt, but I may be wrong here.

divadsn added a comment.Jan 19 2018, 8:02 PM

This comment was removed by divadsn.

All dependencies other than requests are optional.

requirements.txt is a comprehensive list of recommended dependencies. Installing it ensures that the environment can run any pywikibot script properly. I think it's mostly intended for end users, developers might want to install dev-requirements.txt in addition, but it, too, is not a strict requirement. There is also a requests-requirements.txt...

@Dalba ok, that's a point, but now why the revert?

That patch could be fixed with a follow-up, which would be simply adding a |Invalid ISBN found to the regex, why not instead?

In T185317#3913727, @divadsn wrote:

@Dalba ok, that's a point, but now why the revert?

That patch could be fixed with a follow-up, which would be simply adding a |Invalid ISBN found to the regex, why not instead?

IMO, https://gerrit.wikimedia.org/r/#/c/399348/ was more deserved to be merged. It was submitted before your patch and it did not have this issue.
Another issue is the usage of | between the regular expression, which seems too lenient to me. We could try using a regular expression according to available imports. It could simplify the job of finding the changed expression in case upstream changes. I have not thought this through, though.

Just so that you know, I am also doubtful about the whole idea of always using assertRaisesRegex instead of assertRaises. I think that at least sometimes using assertRaisesRegex instead of assertRaises makes tests unnecessarily fragile against changes upstream. If assertRaises was really that bad, python core developers would have deprecated it... I know, this is not the place to discuss this, and I'm fine if others that find it useful continue with that task.

In T185317#3913844, @Dalba wrote:

In T185317#3913727, @divadsn wrote:

@Dalba ok, that's a point, but now why the revert?

That patch could be fixed with a follow-up, which would be simply adding a |Invalid ISBN found to the regex, why not instead?

IMO, https://gerrit.wikimedia.org/r/#/c/399348/ was more deserved to be merged. It was submitted before your patch and it did not have this issue.

But it had other issue and the author wasn't doing anything for a month since that day, that's why I took it up and made mine when working on the task with @jayvdb as mentor.

To the discussion with assertRaises, IMO assertRaisesRegex is fragile only when certain things aren't met like they should be when testing, like requirements or Python versions. If Pywikibot would be required to run on for example newer version of Python 3 with requirements only, it surely would work fine. But that's rather a discussion about supporting a range of versions and requirements.

AssertionError in test_invalid_isbnClosed, ResolvedPublicActions

Description

Details

Related Objects

Event Timeline

AssertionError in test_invalid_isbn
Closed, ResolvedPublic
Actions