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

Add Bot Class #178

Merged
merged 7 commits into from
Oct 20, 2020
Merged

Add Bot Class #178

merged 7 commits into from
Oct 20, 2020

Conversation

xayhewalo
Copy link
Contributor

Provide a useful bot class that future bots can inherit from. Also provide an example script.

tests/test_bots.py Outdated Show resolved Hide resolved
olclient/bots.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@cclauss cclauss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

olclient/bots.py Outdated Show resolved Hide resolved
tests/test_bots.py Outdated Show resolved Hide resolved
@cclauss
Copy link
Collaborator

cclauss commented Aug 26, 2020

Should we drop support for Python 2?

@xayhewalo xayhewalo marked this pull request as ready for review August 27, 2020 00:00
@xayhewalo
Copy link
Contributor Author

xayhewalo commented Aug 27, 2020

I'm stuck on the logging feature of the bot. Here's a truncated log from the failed test:

========================================================================= FAILURES ==========================================================================
_________________________________________________________ TestBots.test_save_when_dry_run_is_false __________________________________________________________

self = <tests.test_bots.TestBots testMethod=test_save_when_dry_run_is_false>, mock_sys_exit = <MagicMock name='exit' id='140352546728312'>

    @patch('olclient.bots.sys.exit')  # so that pytest doesn't exit
    def test_save_when_dry_run_is_false(self, mock_sys_exit):
        save_fn = Mock()
        bot = BaseBot(ol=self.ol, dry_run=False)
        old_changed = copy.deepcopy(bot.changed)
>       bot.save(save_fn)

test_bots.py:57: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../olclient/bots.py:90: in save
    self.logger.info('Modification limit reached. Exiting script.')
/usr/lib/python3.5/logging/__init__.py:1279: in info
    self._log(INFO, msg, args, **kwargs)
/usr/lib/python3.5/logging/__init__.py:1415: in _log
    self.handle(record)
/usr/lib/python3.5/logging/__init__.py:1425: in handle
    self.callHandlers(record)
/usr/lib/python3.5/logging/__init__.py:1487: in callHandlers
    hdlr.handle(record)
/usr/lib/python3.5/logging/__init__.py:855: in handle
    self.emit(record)
/usr/lib/python3.5/logging/__init__.py:1047: in emit
    self.stream = self._open()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <logging.FileHandler object at 0x7fa66038bb70>

    def _open(self):
        """
        Open the current base file with the (original) mode and encoding.
        Return the resulting stream.
        """
>       return open(self.baseFilename, self.mode, encoding=self.encoding)
E       FileNotFoundError: [Errno 2] No such file or directory: '<truncated>/openlibrary-client/olclient/logs/jobs/home/guyrandy/.local/bin/pytest/home/guyrandy/.local/bin/pytest_20200826_165250.log'

/usr/lib/python3.5/logging/__init__.py:1037: FileNotFoundError
------------------------------------------------------------------- Captured stderr call --------------------------------------------------------------------
jobs./home/guyrandy/.local/bin/pytest;INFO    ;2020-08-26 16:52:50,488 Modification limit reached. Exiting script.

I think the problem might be the logger is writing to jobs./home/guyrandy/.local/bin/pytest and pytest expects the file to be at /jobs/home/guyrandy/.local/bin/pytest/home/guyrandy/.local/bin/pytest_20200826_165250.log. Maybe that's not the case. I can't tell the forest from the trees at this point.

Simply intializing the bot used to throw this error, but then i set delay=True on line 53 of bots.py. Which delays the output of the file according to the python docs.

If delay is true, then file opening is deferred until the first call to emit(). By default, the file grows indefinitely.
Still, I'm just guessing at this point

@cclauss
Copy link
Collaborator

cclauss commented Aug 27, 2020

/usr/lib/python3.5/logging/__init__.py:1279: in info

Is the result identical on Python 3.7 or 3.8?

olclient/bots.py Outdated Show resolved Hide resolved
@xayhewalo
Copy link
Contributor Author

Alas, I have found a solution! The problem was job_name = os.path.splitext(sys.argv[0])[0]. Since pytest is called by itself job_name == '' and pytest was getting confused. It had nothing to do with Python 3.5 vs 3.8. The test fails now because of unrelated MARC code.

Paging for @cdrini and @mekarpeles for review

olclient/bots.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok! Some code cleanup things, but this is looking greeeeat!

examples/scripts/making_a_bot.py Outdated Show resolved Hide resolved
olclient/bots.py Outdated Show resolved Hide resolved
olclient/bots.py Outdated Show resolved Hide resolved
olclient/bots.py Outdated Show resolved Hide resolved
tests/test_bots.py Outdated Show resolved Hide resolved
olclient/bots.py Outdated Show resolved Hide resolved
tests/test_bots.py Show resolved Hide resolved
Copy link
Collaborator

@cclauss cclauss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. All comments are OPTIONAL.

examples/scripts/making_a_bot.py Outdated Show resolved Hide resolved
olclient/bots.py Outdated Show resolved Hide resolved
olclient/bots.py Show resolved Hide resolved
olclient/bots.py Outdated Show resolved Hide resolved
olclient/bots.py Outdated Show resolved Hide resolved
tests/test_bots.py Outdated Show resolved Hide resolved
tests/test_bots.py Outdated Show resolved Hide resolved
tests/test_bots.py Outdated Show resolved Hide resolved
tests/test_bots.py Show resolved Hide resolved
@cclauss
Copy link
Collaborator

cclauss commented Oct 2, 2020

I believe that we can comment out this line so the tests pass. ;-)

cdrini and others added 7 commits October 20, 2020 18:49
There might still be some residual code with py2/3 code, but no longer test on/say that we support py 2

Co-authored-by: Christian Clauss <cclauss@me.com>
Co-authored-by: Christian Clauss <cclauss@me.com>
Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's 🚀 this PR! Merging once travis done 👍

@cdrini cdrini merged commit 2d1367c into internetarchive:master Oct 20, 2020
@cdrini
Copy link
Collaborator

cdrini commented Oct 20, 2020

You'll need to cleanup your master (why you should always use branches :) )

git checkout master
git fetch upstream master
git reset --hard upstream/master
git push origin HEAD

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants