Skip to content

Conversation

@dgw
Copy link
Member

@dgw dgw commented May 9, 2021

Description

Fixes #2061, a regression introduced in 5af0eea.

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches

@dgw dgw added Medium Priority Bugfix Generally, PRs that reference (and fix) one or more issue(s) labels May 9, 2021
@dgw dgw added this to the 7.1.0 milestone May 9, 2021
@dgw dgw mentioned this pull request May 9, 2021
Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

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

I think it would be better to use else:

try:
    # do something
except:
    # bot.reply error
except:
    # bot.reply a different error
else:
    # bot.say all is fine

@dgw
Copy link
Member Author

dgw commented May 9, 2021

@Exirel Yeah, per our IRC chatter I did that locally. Haven't pushed yet; sparing Travis until I finish writing the test suite.

@dgw dgw force-pushed the isup-fallthrough-regression branch from d8c072b to 536360f Compare May 9, 2021 08:13
@dgw dgw added the Tests label May 9, 2021
Made simpler thanks to the recent addition of `requests-mock` to Sopel's
dev-requirements.txt file. This replaces an older (late 2020) branch
that I never finished and PR'd.
@dgw dgw force-pushed the isup-fallthrough-regression branch from 536360f to eb60900 Compare May 9, 2021 08:28
@dgw dgw requested a review from Exirel May 9, 2021 08:29
@dgw dgw changed the title isup: fix "looks fine" being sent after "looks down" messages isup: fix double output, and add tests May 9, 2021
Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

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

Is it possible to refactor mocks to reduce the duplicate code in tests? Yes, for sure. Is it necessary to approve that PR? No, it's fine like it is.

Well done on your testing journey, I hope it was enjoyable. As discussed, some tweaks are required in the future to make it even easier, and I probably need to write some more doc about it.

For now, this is a go!

dgw added 2 commits May 9, 2021 16:23
The previous `time.sleep()` approach was suggested on IRC, and I just
ran with it because at the time, it was past 2am.

With a fresh mind, though, and a look at the docstrings for both
`bot.running_triggers` and `bot._update_running_triggers()`, I have
realized the true utility of this property.
@dgw
Copy link
Member Author

dgw commented May 9, 2021

@Exirel Reimplemented fixtures, along with a much more interesting use of bot.running_triggers that I think you'll appreciate:

sopel/sopel/bot.py

Lines 959 to 962 in 559987d

We want to keep track of running triggers, mostly for testing and
debugging purposes. For instance, it'll help make sure, in tests, that
a bot plugin has finished processing a trigger, by manually joining
all running threads.

Early in the process, before figuring out that threading was the problem, I wrongly theorized that some multiple-instance voodoo might be messing up which bot (or at the time, mockbot) got the message vs. which one I checked for message_sent. Since my goal was to get one test working before writing the rest based on it, I removed the fixtures just in case. Clearly, it wasn't necessary, but at the time it seemed like eliminating as many pseudo-magical bits of pytest as possible would help me track down why a seemingly innocuous example directly from one of your own docstrings simply refused to work. 😅

@Exirel
Copy link
Contributor

Exirel commented May 9, 2021

Even better now!

@dgw dgw merged commit c109340 into master May 11, 2021
@dgw dgw deleted the isup-fallthrough-regression branch May 11, 2021 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bugfix Generally, PRs that reference (and fix) one or more issue(s) Medium Priority Tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

isup responds twice

3 participants