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

fix: regression in testing individual modules #1529

Merged

Conversation

HumorBaby
Copy link
Contributor

@HumorBaby HumorBaby commented Mar 30, 2019

Any individual test of a Sopel module (either as python a_module.py or pytest a_module.py; even with full path), leads to this exception:

___________________________________________ test_example_rand_1 ___________________________________________
Traceback (most recent call last):                                                                         
  File "/home/sopel/pkgs/sopel/sopel/test_tools.py", line 113, in test                                     
    bot = MockSopel("NickName", admin=admin, owner=owner)                                                  
  File "/home/sopel/pkgs/sopel/sopel/test_tools.py", line 52, in __init__                                  
    self.channels[channel] = sopel.tools.target.Channel(channel)                                           
AttributeError: 'module' object has no attribute 'target'                                                  

Using sopel.tools.target... in Sopel/test_tools.py throws an error because
sopel.tools does not seem to be aware of target. I expect this is
because target.py requires a class from sopel.tools and this would
lead to circular imports. Need to import sopel.tools.target
separately to avoid this.

pytest tests work fine because it knows of sopel.tools.target by
that name already from traversing the rootdir.

Using `sopel.tools.target...` in `sopel/test_tools.py` an error because
`sopel.tools` does not seem to be aware of `target`. I expect this is
because `target.py` requires a class from `sopel.tools` and this would
leads to circular imports. Need to `import sopel.tools.target`
separately to avoid this.

`pytest` tests work fine because it knows of `sopel.tools.target` by
that name already from traversing the `rootdir`.
@HumorBaby
Copy link
Contributor Author

While digging around in here, I decided to test every module manually to see if anything else was awry. This little exception came up:

sopel/modules/ipython.py                                  
Traceback (most recent call last):                        
  File "Sopel/modules/ipython.py", line 38, in <module>   
    @sopel.module.commands('console')                     
AttributeError: 'module' object has no attribute 'module' 

A separate import sopel.module line is required. While this doesn't fail tests, because there aren't any in this module, the file itself can now stand on its own, without throwing the above exception.

@dgw
Copy link
Member

dgw commented Mar 30, 2019

That ipython error, lol.

Appreciate the work, @HumorBaby! When I have a bit more time I'll look at this in more depth myself. These fixes are good for a patch branch (yes, I'd appreciate you rebasing this on 6.6.x when you get a moment), but I think the module structure itself should be fixed up later on master, for version 7 or maybe 8 (depending on how backward-compatible things can be made).

@HumorBaby HumorBaby changed the base branch from master to 6.6.x March 30, 2019 16:08
@dgw dgw self-requested a review March 30, 2019 16:28
@dgw dgw added the Bugfix Generally, PRs that reference (and fix) one or more issue(s) label Mar 30, 2019
@dgw dgw added this to the 6.6.6 milestone Mar 30, 2019
@dgw dgw added the Tests label Apr 1, 2019
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

Right, so, next steps.

This is a quirk of how Python's imports work. In ipython, for example, import sopel works because at runtime, something else has already imported sopel.module and so that attribute is defined. When running as a standalone file, it's not defined (because nothing has used it yet).

Same for test_tools: At runtime, or during coverage (CI) tests, the name sopel.tools.target has already been imported elsewhere and so the attribute is defined, but it isn't defined in standalone tests.

It might not be reasonable for Sopel, as a project, to just go around trying to force anyone who imports the package sopel (or one of its subpackages) to always load everything under it. We should just be more careful about importing the correct submodules.

@Exirel Your input would be appreciated, but it's not time-sensitive or anything. This PR for 6.6.x is clearly good to go, and we're just talking future stuff for Sopel 7 at this point. 😸

@dgw dgw merged commit 90f1109 into sopel-irc:6.6.x Apr 1, 2019
@HumorBaby HumorBaby deleted the fix-test-regression-individual-modules branch April 1, 2019 13:06
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) Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants