Reduce reference of IO globals #500

mbj · 2015-11-22T00:07:17Z

No description provided.

mbj · 2015-11-22T00:08:39Z

@dkubb Ready for review after #499, sorry for this chain of dependent PRs, but it makes more sense to base this change on top of the global style fix in #499.

mbj · 2015-11-22T14:46:11Z

IMHO this approach seems like a mistake. Naming the option with the same name as the core class should be a clear signal this is less than ideal.

I agree that the state this PR is resulting in is clearly showing a smell. Still I think this state is better than before:

The PR addresses the problem that mutant at various location interacted with IO globals, and the only way to spec these was A): Running the IO operation and observing the mostly global state change B) hooking the IO global via a message expectation, resulting in observable inconsistencies under parallelization (rspec does not do this, but xspec will!)

I think a better approach, here and below, would be to make a simple object that provides a limited interface and uses the core class.

Exactly. That is planned at the next step. But before I want to "lift" the IO globals into the config object, to get a clear "index" on what the wrapping class needs to provide. See this PR as a small intermediary.

Sure, this design allows you to inject the core class in specs, but I think this is taking dependency injection too far. I could be wrong about this, but I think you'll find this approach is a dead-end, and a limited scope wrapper is a much cleaner approach long term.

Its much easier to go from "config object holds globals" to "we have a mutant specific IO object we can pass around" than from "IO globals are everywhere" to the desired state.

There will be some other PRs that reduce more IO global use, the code base still references $stderr and File from classes that do not yet keep state. Fixing these to use the config is the next step, than coming up with the "mutant specific IO wrapper" we discussed.

With that arguments: @dkubb Ready for the next pass.

dkubb · 2015-11-23T07:45:41Z

lib/mutant.rb

@@ -204,7 +204,11 @@ class Config
 integration: Integration::Null,
 isolation: Mutant::Isolation::Fork,
 jobs: Mutant.ci? ? CI_DEFAULT_PROCESSOR_COUNT : ::Parallel.processor_count,
+ kernel: Kernel,


So you've audited the code to make sure it doesn't refer to anything outside of the Mutant namespace, besides here of course?

No direct references to Kernel exist inside the mutant namespaces anymore, correct. But other IO globals are still referenced and need future iterations, this PR is only the first step.

I extracted this PR out of a bigger change, as other changes need refactorings to be applied first.

dkubb · 2015-11-23T07:50:42Z

@mbj I finished a pass over this code.

mbj · 2015-11-23T07:57:03Z

@dkubb Ready for the next pass.

dkubb · 2015-11-24T05:22:12Z

lib/mutant/cli.rb

@@ -153,14 +164,14 @@ def add_debug_options(opts)
 end
 opts.on('--version', 'Print mutants version') do
 puts("mutant-#{VERSION}")
- Kernel.exit(EXIT_SUCCESS)
+ config.kernel.exit(EXIT_SUCCESS)


Isn't the default status code for Kernel.exit success? If so, then the EXIT_SUCCESS argument isn't required.

true, good catch

Is it possible to remove the EXIT_SUCCESS constant altogether?

dkubb · 2015-11-24T05:45:29Z

@mbj I finished a pass over this PR.

mbj · 2015-12-20T18:01:39Z

Depends on #508

backus · 2015-12-24T10:20:14Z

I noted remaining globals as I looked through mutant. I don't think this is exhaustive and it doesn't cover implicit usage like with Kernel:

Mutex and ConditionVariable in mailbox.rb
File and IO references in isolation.rb
Object references in expression/method.rb, expression/methods.rb, expression/namespace.rb
Marshall in isolation.rb
ObjectSpace in env/bootstrap.rb
Open3 in cli/tput.rb
OptionParser in cli.rb
Regexp in expression/namespace and mutant/mutator/node/named_value/variable_assignment.rb
Digest::SHA1 in mutation.rb
Set in mutator, parallel/master, and zombifier.rb
StringIO in reporter/cli/format
Time in env.rb, integration/rspec.rb, and runner/sink.rb

mbj · 2015-12-24T12:48:49Z

@backus Yeah, I'm ware this PR is not exhaustive. Its intend to slice small portions each time I find motivations. In xspec I do not want to support setting message expectations on anything else than doubles hence over the time we need to eradicate all the globals we use and set message expectations on.

So not all of the globals in your list have to be changed, as there are AFAIK no message expectations on for example Set.

backus · 2015-12-25T18:41:48Z

Yeah, I'm ware this PR is not exhaustive.

Ah yeah I understood that. I just wanted to have some idea of what is still left and I also wanted to know if you are actually planning to kill all of these constant references too.

mbj · 2015-12-25T19:16:47Z

I also wanted to know if you are actually planning to kill all of these constant references too.

Longterm yes.Currently I cut the iteration into a small one to get it merged as an iterative improvement before it grew to big. I'll do followups to focus on the globals I've to place message expectations on. This is the blocker for making mutant hosted on xspec.

I just wanted to have some idea of what is still left

NP

dkubb · 2015-12-28T21:37:20Z

@mbj I will be ready to merge this once #508 is merged.

dkubb · 2015-12-31T18:48:59Z

lib/mutant/config.rb

@@ -13,7 +13,11 @@ class Config
 :includes,
 :isolation,
 :jobs,
+ :kernel,


@mbj not a blocker, but should this Array of Symbols be defined using %i[...]?

Reduce reference of IO globals

Reduce direct reference of globals

9ab35de

mbj force-pushed the fix/reduce-globals branch 2 times, most recently from 8d6cda4 to 3cea008 Compare November 22, 2015 00:08

mbj force-pushed the fix/reduce-globals branch from 661f686 to b41f1fe Compare November 22, 2015 15:27

mbj added the enhancement label Nov 22, 2015

dkubb force-pushed the fix/reduce-globals branch 2 times, most recently from c4c887e to ba07692 Compare November 23, 2015 07:30

dkubb assigned mbj Nov 23, 2015

dkubb reviewed Nov 23, 2015
View reviewed changes

mbj force-pushed the fix/reduce-globals branch from ba07692 to ece8edd Compare November 23, 2015 07:56

dkubb reviewed Nov 24, 2015
View reviewed changes

dkubb force-pushed the fix/reduce-globals branch 2 times, most recently from ba22f2b to 8d4c326 Compare November 24, 2015 05:35

mbj force-pushed the fix/reduce-globals branch 5 times, most recently from 432f03c to 540edf8 Compare December 19, 2015 22:02

mbj force-pushed the fix/reduce-globals branch 2 times, most recently from 59e81fb to 5262a1e Compare December 20, 2015 19:28

mbj force-pushed the fix/reduce-globals branch 6 times, most recently from e74414f to 9194723 Compare December 21, 2015 01:32

mbj force-pushed the fix/reduce-globals branch from 9194723 to fe1deba Compare December 31, 2015 14:26

dkubb force-pushed the fix/reduce-globals branch from 8e1c08e to f45d276 Compare December 31, 2015 18:40

dkubb mentioned this pull request Dec 31, 2015

Add test minitest integration #445

Merged

dkubb reviewed Dec 31, 2015
View reviewed changes

dkubb force-pushed the fix/reduce-globals branch from f45d276 to 9ab35de Compare December 31, 2015 19:01

dkubb added a commit that referenced this pull request Dec 31, 2015

Merge pull request #500 from mbj/fix/reduce-globals

b2207da

Reduce reference of IO globals

dkubb merged commit b2207da into master Dec 31, 2015

dkubb deleted the fix/reduce-globals branch December 31, 2015 19:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce reference of IO globals #500

Reduce reference of IO globals #500

mbj commented Nov 22, 2015

mbj commented Nov 22, 2015

mbj commented Nov 22, 2015

dkubb Nov 23, 2015

mbj Nov 23, 2015

dkubb commented Nov 23, 2015

mbj commented Nov 23, 2015

dkubb Nov 24, 2015

mbj Nov 24, 2015

dkubb Nov 24, 2015

dkubb commented Nov 24, 2015

mbj commented Dec 20, 2015

backus commented Dec 24, 2015

mbj commented Dec 24, 2015

backus commented Dec 25, 2015

mbj commented Dec 25, 2015

dkubb commented Dec 28, 2015

dkubb Dec 31, 2015

Reduce reference of IO globals #500

Reduce reference of IO globals #500

Conversation

mbj commented Nov 22, 2015

mbj commented Nov 22, 2015

mbj commented Nov 22, 2015

dkubb Nov 23, 2015

Choose a reason for hiding this comment

mbj Nov 23, 2015

Choose a reason for hiding this comment

dkubb commented Nov 23, 2015

mbj commented Nov 23, 2015

dkubb Nov 24, 2015

Choose a reason for hiding this comment

mbj Nov 24, 2015

Choose a reason for hiding this comment

dkubb Nov 24, 2015

Choose a reason for hiding this comment

dkubb commented Nov 24, 2015

mbj commented Dec 20, 2015

backus commented Dec 24, 2015

mbj commented Dec 24, 2015

backus commented Dec 25, 2015

mbj commented Dec 25, 2015

dkubb commented Dec 28, 2015

dkubb Dec 31, 2015

Choose a reason for hiding this comment