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

Lexical strict mode for mockmodule makes it difficult to have global strict mode. #58

Open
toddr opened this issue Jul 18, 2022 · 4 comments

Comments

@toddr
Copy link
Contributor

toddr commented Jul 18, 2022

In #53, an issue was raised that a global strict mode was a problem for some Mojo tests. The pull request submitted in #54 got rid of global strict and replaced it with a lexical only strict mode for Test::MockModule.

@DrHyde, We needed this global strict mode which we used in a policy to detect if people were failing to set strict mode for their import.

Looking at the pull request, I want to suggest an alternative. I would like to put back the global strict mode but honor a lexical "nostrict" mode when specified. Would this work for you @DrHyde ?

@DrHyde
Copy link
Contributor

DrHyde commented Jul 19, 2022

I'd be fine with that. Alternatively, I could re-jig my patch to only have any effect if you use Test::MockModule qw(lexically) or something like that. The setting of whether to be lexical or not would of course be lexical itself, so global use shouldn't be affected.

@atoomic
Copy link
Contributor

atoomic commented Aug 2, 2022

Before providing any code, here is a suggestion.

What about requesting for global or lexical mode on import?

By default we could promote global.
If both lexical and global strictness are enabled, then we croak and notify the user of incompatible usage of the two modes.

use Test::MockModule 'strict';                       # global is the default mode when not specified
use Test::MockModule 'strict' => 'global';    # you can also set the global mode
# or
use Test::MockModule 'strict' => 'lexical'; # or if you prefer you can set the lexically mode

Using both mode will die

# 
like dies {
	use Test::MockModule 'strict' => 'global';
	# ... then later
	use Test::MockModule 'strict' => 'lexical';
}, qr{Incompatible usage of Test::MockModule strict modes 'lexical' and 'global'. Use one or the other.}

@DrHyde would that work and match what you have in mind?

@atoomic
Copy link
Contributor

atoomic commented Aug 2, 2022

@DrHyde I'm also wondering why #54 was added in the first place.

It looks like that global strict mode was a problem in your case, and you wanted to disable it in some targeted locations.
But why not turning off strict mode (globally) in your case?

The only point of strict is to prevent developers from calling mock or unmock accidentally.
By forcing developers to use redefine instead we can catch issues during code refactor and improve test suite quality.

I'm balancing the pros/cons of reverting #54 or providing a more complex solution to accommodate the global/lexical mode.
But first, I really need to understand the need for #54.

Thanks for providing more details.

@DrHyde
Copy link
Contributor

DrHyde commented Aug 16, 2022

We wanted to lexicalize it at work so we could incrementally improve our code, using strict mode for new tests while not breaking existing tests in the same file. However, if it's causing problems then by all means revert it and we can pin the version we're using until I can come up with a variation that only lexicalizes when it's asked for.

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

No branches or pull requests

3 participants